chore: move disks into the service #1336
Conversation
WalkthroughThis pull request enhances the GraphQL schema and refactors disk handling. The schema now includes a new Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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 (
|
elibosley
force-pushed
the
chore/move-disks-to-service
branch
from
950b08b to
280c096
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts (1)
24-29: Consider more specific mocks instead ofas any.Casting to
anymay hide type errors. Over time, this might become a source of bugs. A more fine-grained approach using interfaces or more targeted partial mocks could catch potential type mismatches earlier.- const mockExeca = execa as any; - const mockBlockDevices = blockDevices as any; - const mockDiskLayout = diskLayout as any; - const mockBatchProcess = batchProcess as any; + const mockExeca = execa as unknown as MockedFunction<typeof execa>; + const mockBlockDevices = blockDevices as unknown as MockedFunction<typeof blockDevices>; + const mockDiskLayout = diskLayout as unknown as MockedFunction<typeof diskLayout>; + const mockBatchProcess = batchProcess as unknown as MockedFunction<typeof batchProcess>;api/src/unraid-api/graph/resolvers/disks/disks.service.ts (1)
129-155: Selective batching for temperature retrieval.Using the
batchProcessfunction to handle per-disk temperature checks is a good pattern to manage concurrency. Passingtemperature: falsedirectly skips these overheads, which is efficient.Consider a configurable batch size to fine-tune performance in large-scale environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ 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/generated-schema.graphql(3 hunks)api/src/core/modules/get-disks.ts(0 hunks)api/src/graphql/schema/types/array/array.graphql(2 hunks)api/src/graphql/schema/types/disks/disk.graphql(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
💤 Files with no reviewable changes (1)
- api/src/core/modules/get-disks.ts
🧰 Additional context used
🧬 Code Definitions (5)
api/src/unraid-api/graph/resolvers/disks/disks.module.ts (1)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
Module(38-76)
api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts (1)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1)
disks(18-21)
api/src/unraid-api/graph/resolvers/disks/disks.service.ts (2)
api/src/graphql/generated/api/types.ts (1)
Disk(509-530)api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1)
disks(18-21)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1)
api/src/graphql/generated/api/types.ts (2)
Resolver(1976-1976)Query(1211-1264)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
api/src/graphql/generated/api/types.ts (1)
Disk(509-530)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (16)
api/src/graphql/schema/types/disks/disk.graphql (1)
54-55: Added ext4 and ntfs filesystem typesThe addition of
ext4andntfsto theDiskFsTypeenum extends filesystem support beyond the existing options. This is appropriate as these are common filesystem types that may be present on disk partitions.api/src/unraid-api/graph/resolvers/disks/disks.module.ts (1)
1-10: Good modularization of disk functionalityCreation of a dedicated
DisksModulefollowing NestJS best practices properly encapsulates disk-related functionality. The module correctly provides and exports the necessary components.api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
14-14: Proper integration of DisksModuleThe resolver module now correctly imports and uses the
DisksModuleinstead of directly including theDisksResolverin its providers. This follows NestJS module composition patterns.Also applies to: 39-39
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1)
6-6: Improved dependency injection patternThe resolver now uses proper dependency injection by receiving the
DisksServicethrough its constructor and calling the service method instead of directly using an imported function. This improves testability and adheres to separation of concerns.Also applies to: 10-11, 19-19
api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts (3)
14-22: Good practice mocking external dependencies.The approach of globally mocking
execa,systeminformation, and@app/utils.jshelps ensure minimal side effects during testing. This is a robust setup, ensuring the tests remain deterministic.
205-246: Excellent coverage of “no temperature” scenario.The test verifies that the code handles the scenario where temperature is intentionally omitted (
options.temperature = false), ensuring no needlesssmartctlcalls. This helps confirm that your method logic and mocks are working as expected.
248-323: Broad coverage of temperature retrieval logic.Your test suite exhaustively checks normal, error, and partial-failure cases. This thoroughness helps confirm resiliency, especially verifying how the system handles disks that fail
smartctlcalls.api/src/unraid-api/graph/resolvers/disks/disks.service.ts (2)
16-43: Graceful error handling ingetTemperature.Catching errors and returning
-1is a solid approach for resilience. It prevents unexpected crashes in the service ifsmartctlfails.
45-124: Robust disk parsing logic.Mapping file system types and interface types to enum values ensures type safety and clarity. Filtering out unknown fsTypes is also a helpful safeguard to avoid polluting your data with unsupported partitions.
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (2)
11-15: Mocking the DisksService.The straightforward mock structure uses
vi.fn()to replacegetDisks, which keeps tests isolated without external side effects. This is a clear pattern for verifying resolver behavior.
42-83: Correct usage of service call in resolver.These tests confirm that the resolver indeed invokes
getDisks({ temperature: true }), aligning with your design to always include temperature data by default. This setup ensures well-defined interactions between resolver and service.api/generated-schema.graphql (3)
122-124: New Field Addition:colorin ArrayDisk
The new fieldcolor: ArrayDiskFsColorin theArrayDisktype enriches the disk representation. Please ensure that all resolver logic and related services are updated to include this field.
191-212: Enum Update:ArrayDiskFsColorEnhancements
The updatedArrayDiskFsColorenum now provides a more granular mapping of disk statuses—with clear descriptions for states such asblue_blink,blue_on,green_blink,yellow_blink, etc. Please verify that these new values are consistently mapped in both the frontend and backend services.
543-547: Enum Expansion:DiskFsTypeUpdated
Addingext4andntfsto theDiskFsTypeenum broadens filesystem support. Confirm that downstream disk partition parsing logic correctly handles these new file system types.api/src/graphql/schema/types/array/array.graphql (2)
177-179: New Field Addition:colorin ArrayDisk
The addition of thecolor: ArrayDiskFsColorfield in theArrayDisktype aligns with the overall schema enhancements. Ensure that the disk data retrieval logic now includes the disk color information.
195-214: Enum Enhancement:ArrayDiskFsColorRevisions
The revisedArrayDiskFsColorenum introduces refined statuses (e.g.,green_blink,blue_on,blue_blink, etc.) with updated descriptions that improve clarity. Double-check that this updated enum is reflected in resolver logic and schema documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/disks/disks.service.ts (2)
45-86: Handle edge cases in partition matching logic.The condition
partition.name.startsWith(disk.device.split('/dev/')[1])relies on/dev/consistently appearing indisk.device. It's possible fordisk.deviceto lack/dev/in edge cases (e.g., rarely named devices). Consider adding checks or fallback logic to handle scenarios where/dev/is missing—or using a more robust approach to correlate partitions to their parent disk.- .filter((partition) => partition.name.startsWith(disk.device.split('/dev/')[1])) + .filter((partition) => { + const deviceName = disk.device.replace('/dev/', ''); + return partition.name.startsWith(deviceName); + })
129-155: Note on performance when collecting disk temperatures.Each disk’s temperature retrieval spawns a separate
smartctlprocess. For systems with a large number of disks, consider implementing concurrency limits or a parallel batch strategy to optimize performance and avoid overwhelming resources.api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
42-74: Add a test case for temperature disabled scenario.Currently, the tests mock
getDiskscalls withtemperature: true. For completeness, consider adding a test to confirm thatresolver.disks({ temperature: false })also works correctly, ensuring coverage of theifbranch in the service.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ 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 (11)
api/generated-schema.graphql(3 hunks)api/src/core/modules/get-disks.ts(0 hunks)api/src/core/modules/index.ts(0 hunks)api/src/graphql/schema/types/array/array.graphql(2 hunks)api/src/graphql/schema/types/disks/disk.graphql(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
💤 Files with no reviewable changes (2)
- api/src/core/modules/index.ts
- api/src/core/modules/get-disks.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- api/src/unraid-api/graph/resolvers/disks/disks.module.ts
- api/src/graphql/schema/types/disks/disk.graphql
- api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts
- api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts
- api/generated-schema.graphql
- api/src/graphql/schema/types/array/array.graphql
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
api/src/graphql/generated/api/types.ts (1)
Disk(509-530)
⏰ 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 (3)
api/src/unraid-api/graph/resolvers/disks/disks.service.ts (1)
16-43:❓ Verification inconclusive
Consider validating disk.device to mitigate potential shell injection.
Here, the
execacall tosmartctlutilizesdisk.deviceas a command argument without explicit sanitization. Ifdisk.deviceever comes from untrusted sources, there may be a risk of command injection. Even though the current usage likely sourcesdisk.devicefrom the OS enumerations, you may want to confirm or add a sanitization step for robust security.Below is a basic script to verify if
disk.devicestrings can ever originate from user input:
🏁 Script executed:
#!/bin/bash # Search references of "disk.device" usage to confirm whether it may be user-provided. rg "disk\.device" -A 3Length of output: 1291
Action: Confirm and Sanitize
disk.deviceInputThe current implementation of
getTemperaturepassesdisk.devicedirectly to thesmartctlcommand. Although our search indicates thatdisk.deviceis populated in a controlled manner (e.g., from OS enumerations in bothdisks.service.tsandget-array-data.ts), there’s no explicit validation or sanitization in place. Please ensure that:
disk.deviceis strictly validated: Confirm it consistently follows an expected device format (e.g., starts with/dev/) before using it as a command argument.- Document its integrity: If the value is guaranteed to be from a trusted source, add a clarifying comment to that effect to ease future reviews.
Addressing this will help safeguard against any unforeseen injection risks if the source ever becomes less controlled.
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
14-39: Clean and correct module import.Using
DisksModulein theimportsarray instead of directly providingDisksResolveris a well-structured NestJS practice. This modular approach boosts maintainability and keeps disk-related logic neatly encapsulated.api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
76-83: Thorough usage of mocks.The tests properly reset mocks and assert the expected calls. This approach helps ensure tests are isolated and reproducible. The current structure is good and no further concerns are apparent here.
elibosley
force-pushed
the
chore/move-disks-to-service
branch
from
280c096 to
b42f65e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
api/src/unraid-api/graph/resolvers/disks/disks.service.ts (6)
16-21: Consider removing redundant catch inside the try/catch block.
You already have atry/catchwrappingexeca(...), yet you also use.catch(...)on theexecacall. The.catch(...)sets an empty string, but the enclosingtry/catchwould also handle errors. You might simplify this flow to rely only on one approach.-const stdout = await execa('smartctl', ['-A', disk.device]) - .then(({ stdout }) => stdout) - .catch(() => ''); +const { stdout: commandOutput } = await execa('smartctl', ['-A', disk.device]); +const stdout = commandOutput;
23-23: Avoid mutating arrays with.splice().
Using.splice()here mutateslines. Instead, consider.slice()to avoid side effects and improve clarity.-const fields = lines.splice(lines.indexOf(header) + 1, lines.length); +const fields = lines.slice(lines.indexOf(header) + 1);
29-29: Log absence of temperature fields.
Whenfieldis not found, you silently return-1. Consider adding a log statement so users know the temperature field was missing, aiding troubleshooting.-if (!field) { - return -1; +if (!field) { + graphqlLogger.warn(`No temperature field found for device ${disk.device}`); + return -1; }
40-43: Check for partial results before setting default temperature.
Currently, you return-1for any error, including partial or unexpected command results. You might differentiate between a real error (command failure) and partial data to provide more nuanced error handling.
130-131: Remove unusedvarsvariable if it's not needed.
varsis assigned fromgetters.emhttp().varbut is never read or used, which may confuse future maintainers.-const vars = getters.emhttp().var; -
133-155: Factor out temperature flag logic to reduce duplication.
There are two blocks that retrieve partitions and disk layouts, differing only by thetemperatureflag. You can unify the code and pass the temperature option toparseDisk()in a single pass for readability and maintenance.
📜 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 (11)
api/generated-schema.graphql(3 hunks)api/src/core/modules/get-disks.ts(0 hunks)api/src/core/modules/index.ts(0 hunks)api/src/graphql/schema/types/array/array.graphql(2 hunks)api/src/graphql/schema/types/disks/disk.graphql(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
💤 Files with no reviewable changes (2)
- api/src/core/modules/index.ts
- api/src/core/modules/get-disks.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- api/src/graphql/schema/types/disks/disk.graphql
- api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts
- api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts
- api/src/unraid-api/graph/resolvers/disks/disks.module.ts
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- api/src/graphql/schema/types/array/array.graphql
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
api/src/graphql/generated/api/types.ts (1)
Disk(509-530)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
api/generated-schema.graphql (3)
122-124: New "color" Field Added to ArrayDisk
The addition ofcolor: ArrayDiskFsColorto theArrayDisktype enriches disk metadata by explicitly representing disk color status. Ensure that the corresponding resolver and any client-side consumers are updated to correctly handle this new field.
191-222: Enhanced ArrayDiskFsColor Enum Values
The updatedArrayDiskFsColorenum now provides a finer granularity of disk statuses by introducing values such asblue_blink,blue_on,green_blink,grey_off,red_on, andyellow_blink, while the outdatedgreen_offandyellow_offvalues have been removed. Verify that all parts of the application consuming this enum correctly interpret the new statuses and that any existing references to the removed values are updated.
543-550: Expanded DiskFsType Enum with ext4 and ntfs
Addingext4andntfsto theDiskFsTypeenum broadens the supported file systems. Please ensure that disk parsing and validation logic, as well as any mappings in related services, correctly integrate these new file system types.api/src/unraid-api/graph/resolvers/disks/disks.service.ts (1)
53-88: Revisit ignoring unknown filesystem types.
You skip partitions whosefsTypeis not recognized. Consider an explicit enum member for unknown FS types to preserve partial data and avoid silently dropping partitions.Could you confirm whether ignoring partitions with unknown FS types is desired behavior, or do you need to track these partitions under a special “UNKNOWN” fsType?
-default: - mappedFsType = undefined; // Handle unknown types as undefined +default: + mappedFsType = DiskFsType.UNKNOWN;api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
76-83: Tests are looking good.
You have proper coverage of calls toresolver.disks(). The mock-based approach effectively ensures the resolver and service are interacting as expected. Great job!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
api/src/__test__/setup/mock-fs-setup.ts (1)
12-18: Consider adding Buffer support to the readFile mock.The current implementation returns content directly as a string. In real-world scenarios,
readFilereturns a Buffer by default unless encoding is specified.readFile: vi.fn().mockImplementation((path, options) => { const content = mockFileSystem.get(path.toString()); if (content === undefined) { return Promise.reject(new Error(`File not found: ${path}`)); } - return Promise.resolve(content); + // Check if options specify an encoding + if (options && typeof options === 'object' && options.encoding) { + return Promise.resolve(content); + } + // Default to Buffer when no encoding is specified + return Promise.resolve(Buffer.from(content)); }),api/src/unraid-api/auth/cookie.service.spec.ts (2)
3-5: Remove unnecessary imports of real functions.Since you're mocking
writeFileandemptyDir, the direct imports of these real functions are unnecessary and potentially confusing.- import { writeFile } from 'node:fs/promises'; - import { emptyDir } from 'fs-extra'; + import { afterAll, beforeAll, describe, it, vi } from 'vitest';
54-61: Update function to use mocked writeFile.The
makeSessionfunction uses the importedwriteFilefunction, which is now mocked. For clarity, consider usingvi.mockedto make this explicit.function makeSession(sessionId: string, cookieService: CookieService = service) { const path = cookieService.getSessionFilePath(sessionId); - return writeFile( + return vi.mocked(writeFile)( path, `unraid_login|i:1736523078;unraid_user|s:4:"root";locale|s:0:"";buildDate|s:8:"20241202";`, 'ascii' ); }api/src/store/root-reducer.ts (1)
32-40: Consider adding stricter typing for state and action.
The function signature usesany, which might mask potential type issues. If possible, use your RootState type or Dispatch type for stronger type safety.api/src/store/modules/config.ts (1)
80-82: Lazy loading dependencies.
Lazy loadingpubsub,stopPingTimeoutJobs, andGraphQLClientinsidelogoutUserhelps avoid loading them when not needed. Keep an eye on potential performance hits if logout is called frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (16)
api/src/__test__/common/allowed-origins.test.ts(1 hunks)api/src/__test__/core/utils/misc/get-key-file.test.ts(2 hunks)api/src/__test__/core/utils/shares/get-shares.test.ts(1 hunks)api/src/__test__/setup.ts(1 hunks)api/src/__test__/setup/mock-fs-setup.ts(1 hunks)api/src/__test__/setup/store-reset.ts(1 hunks)api/src/__test__/store/modules/config.test.ts(2 hunks)api/src/__test__/store/modules/registration.test.ts(3 hunks)api/src/common/allowed-origins.ts(2 hunks)api/src/store/actions/reset-store.ts(1 hunks)api/src/store/index.ts(1 hunks)api/src/store/modules/config.ts(2 hunks)api/src/store/modules/registration.ts(1 hunks)api/src/store/root-reducer.ts(1 hunks)api/src/unraid-api/auth/cookie.service.spec.ts(1 hunks)api/vite.config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/src/test/setup.ts
🧰 Additional context used
🧬 Code Definitions (7)
api/src/__test__/core/utils/shares/get-shares.test.ts (2)
api/src/store/index.ts (1)
store(6-12)api/src/store/modules/emhttp.ts (1)
loadStateFiles(133-152)
api/src/__test__/setup/store-reset.ts (2)
api/src/store/index.ts (1)
store(6-12)api/src/store/actions/reset-store.ts (1)
resetStore(7-7)
api/src/store/root-reducer.ts (3)
api/src/store/modules/config.ts (1)
configReducer(348-348)api/src/store/modules/registration.ts (1)
registrationReducer(69-69)api/src/store/actions/reset-store.ts (1)
resetStore(7-7)
api/src/__test__/store/modules/config.test.ts (1)
api/src/core/pubsub.ts (1)
pubsub(24-24)
api/src/__test__/common/allowed-origins.test.ts (3)
api/src/store/index.ts (1)
store(6-12)api/src/store/modules/emhttp.ts (1)
loadStateFiles(133-152)api/src/store/modules/config.ts (1)
loadConfigFile(121-166)
api/src/__test__/store/modules/registration.test.ts (1)
api/src/store/index.ts (1)
getters(18-29)
api/src/store/index.ts (1)
api/src/store/root-reducer.ts (1)
rootReducer(32-40)
⏰ 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 (22)
api/src/__test__/core/utils/misc/get-key-file.test.ts (2)
47-47: Status expectation updated correctly.The expected registration status when the key location is empty has been changed from
LOADEDtoUNLOADED, which aligns with the current behavior. This change ensures the test accurately reflects the system's state when no key file is available.
56-56: Added registration key loading to complete the test flow.The test has been improved by explicitly loading the registration key before checking its status. This ensures a complete and accurate test of the key file retrieval process when a key location exists.
Also applies to: 59-59
api/src/__test__/core/utils/shares/get-shares.test.ts (1)
212-212: Good addition to ensure proper test setupAdding this line ensures that the state is properly loaded before running the test assertions. This improves test reliability by making each test independent and not relying on state potentially set up by previous tests. This change aligns with best practices for test isolation.
api/src/common/allowed-origins.ts (1)
94-94: Improvement to explicitly pass state parameterThis change explicitly passes the state parameter to
getLocalAccessUrlsForServerinstead of relying on its default value. This ensures that the same state instance is used consistently throughout the entire chain of function calls withingetAllowedOrigins, preventing potential issues where the state might be retrieved multiple times.api/src/__test__/common/allowed-origins.test.ts (3)
1-1: Simplified importsThe imports have been streamlined to only include what's actually used in the test. This improves readability and follows best practices for clean code.
8-8: Updated testing importsThe imports from vitest are more focused now, only importing what's needed. This is a good practice as it makes dependencies clearer.
12-13: Added unwrap() to properly handle Promise resultsAdding
.unwrap()to the store dispatch calls ensures that any errors during dispatch are properly propagated, which makes test failures more obvious and easier to debug.api/src/__test__/store/modules/config.test.ts (3)
1-1: Good addition ofbeforeEachimportAdding the
beforeEachimport from Vitest allows for better test isolation by resetting mock state between tests.
13-34: Good mock implementation with ES Module compatibilityThe updated mock provides better ES module compatibility by including
__esModule: trueand adefaultexport that mirrors the named exports. This ensures the mock works correctly with both import styles.
36-42: Good practice for mock lifecycle managementStoring the mock function in a constant and clearing it before each test is a best practice that ensures test isolation and prevents state leakage between tests.
api/vite.config.ts (1)
146-146:❓ Verification inconclusive
Test isolation change - impact on testing strategy
Changing
isolatefromfalsetotruemeans tests will now run in isolation. This aligns with the architectural change of moving disk functionality into a service, as tests should be independent and not rely on shared state.This change improves test reliability but may affect tests that previously depended on shared context between test cases.
🏁 Script executed:
#!/bin/bash # Check if there are any existing tests that might rely on shared state # Looking for tests that don't clean up resources or that use global mocks without resetting # Search for tests that might use shared state echo "Searching for tests that might rely on shared state between test runs..." rg -l "beforeAll|afterAll" --glob "**/*.spec.ts" | grep -v "setup.ts" # Look for global mock patterns that might not get reset between tests echo "Checking for global mocks that might need explicit clearing..." rg -l "jest.mock|vi.mock" --glob "**/*.spec.ts"Length of output: 1363
Action: Verify and Update Tests for Shared-State Dependencies
The change to
isolate: truemeans tests will now run completely isolated, which is in line with our move to a service-based architecture. However, our recent search revealed several test files using lifecycle methods and global mocks that may rely on shared state:
Shared lifecycle methods:
api/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.tsapi/src/unraid-api/graph/resolvers/vms/vms.service.spec.tsapi/src/unraid-api/auth/cookie.service.spec.tsGlobal mocks in tests:
Multiple files (e.g., tests underapi/src/unraid-api/auth/andapi/src/unraid-api/graph/) usejest.mock/vi.mock, which might not reset automatically when tests run in isolation.Please review these tests to ensure they correctly initialize and clean up their context. This will help avoid unexpected failures if there were previously implicit dependencies on shared state.
api/src/__test__/setup/mock-fs-setup.ts (1)
1-45: Well-structured mock file system for testing purposes.This implementation provides a clean way to mock file system operations across tests. The approach using a Map to simulate file storage is effective.
api/src/store/modules/registration.ts (1)
69-69: Clean export addition for registrationReducerGood addition of the reducer export. This allows the reducer to be imported by the root reducer, supporting the store restructuring mentioned in the PR objectives.
api/src/__test__/store/modules/registration.test.ts (2)
3-3: Good update to import getters alongside storeThis change correctly imports the getters object that's now being used to access store state.
10-10: Proper refactoring to use getters instead of direct state accessThe changes consistently replace direct state access via
store.getState().registrationwith the new getters approach usinggetters.registration(). This follows the store architecture updates and makes the tests more maintainable if the state structure changes.Also applies to: 20-20, 45-45
api/src/store/actions/reset-store.ts (1)
1-7: Well-implemented store reset actionGood implementation of the reset action with clear documentation. This follows Redux best practices by using
createActionand will be useful for maintaining clean state between tests.api/src/__test__/setup/store-reset.ts (1)
1-8: Good test setup for store resetsThe
beforeEachhook properly dispatches the reset action before each test, ensuring a clean store state. This implementation will help prevent test state pollution and improve test reliability.api/src/store/root-reducer.ts (3)
1-13: Looks good on imports.
All imported modules are referenced appropriately to build out the combined reducer.
15-18: Helpful documentation.
The doc comment precisely explains the purpose of the root reducer and the reset mechanism.
19-30: Appropriate usage of combineReducers.
Each slice reducer is properly included. This setup is clear and maintainable.api/src/store/modules/config.ts (1)
67-67: Dynamic import introduced.
Switching to dynamic import forpubsubandPUBSUB_CHANNELis a reasonable approach, especially if this code path is rarely invoked, as it can reduce initial load time.api/src/store/index.ts (1)
4-4: Root reducer integration approved.
Replacing individual reducer imports with a singlerootReducersimplifies store setup and keeps the state management logic in one place.Also applies to: 7-7
| 'src/__test__/setup/env-setup.ts', | ||
| 'src/__test__/setup/keyserver-mock.ts', | ||
| 'src/__test__/setup/config-setup.ts', | ||
| 'src/__test__/setup.ts', |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consolidated test setup files
The test setup has been streamlined to use a single setup.ts file instead of multiple setup files. This is a good practice that centralizes test configuration and makes maintenance easier.
Make sure that all necessary setup configurations from the previous files have been properly migrated to this single file.
🏁 Script executed:
#!/bin/bash
# Verify that the new setup.ts file contains all necessary setup configurations
# Find the new setup file
echo "Checking the content of the new setup file:"
cat src/__test__/setup.ts
# Check for any previous setup files that might contain important configurations
echo "Looking for previous setup files that might need migration:"
find src -name "*setup*.ts" -not -path "src/__test__/setup.ts" | xargs catLength of output: 421
Consolidated Test Setup File Missing
The configuration in api/vite.config.ts expects the src/__test__/setup.ts file. However, our verification indicates that this file does not exist in the repository. Please ensure that the consolidated test setup file is added or update its reference if it has been relocated or renamed. Also, confirm that all necessary configurations from previous setup files have been migrated appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
43-75: Add a unit test for temperature disabled.
A previous reviewer suggested adding a scenario to confirm correct behavior whentemperatureis explicitly disabled (e.g.,disks(false)). This remains unaddressed.
🧹 Nitpick comments (10)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (2)
11-21: Removed jGrowl code block.
The legacy notification logic was taken out, leaving a blank line. Ensure all references to jGrowl have been removed from the codebase and verify the new notification mechanism is functioning correctly.
60-68: Old bell color updates removed.
This eliminates the old alert color logic. Ensure that the new toaster or other UI elements adequately signal alert states if needed.api/src/unraid-api/graph/resolvers/disks/disks.service.ts (1)
13-37: Check exception handling ingetTemperature.
The function gracefully handles missing data by returningnull. Consider logging failures for troubleshooting or adding a short message to help with debugging temperature retrieval issues.api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (3)
52-58: Added new CSS references.
The multiple stylesheet references for Base, Dynamix, UI, etc. look consistent. Validate these are loaded in the correct order to prevent styling conflicts.
81-88: Conditional background color styling.
Applying#<?=$backgnd?>is straightforward. Ensure dynamic values are sanitized if they can come from user input.
1309-1310: Restarting subscribers on focus.
This logic helps restore live updates. Consider whether you need a delayed or queued approach for heavier data loads after resuming.api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (2)
11-15: Optional: Add coverage for error scenarios in service mocks.
You might want to add a test verifying behavior when service methods throw exceptions or reject promises, ensuring robust error handling paths in the resolver.
87-119: Consider negative or absent temperature scenarios.
Testing edge cases (e.g., negative temperature, missing disk device, or service errors) can further strengthen reliability.🧰 Tools
🪛 GitHub Check: Test API
[failure] 110-110:
Delete············
[failure] 112-112:
Delete············
[failure] 114-114:
Delete············🪛 GitHub Check: Build API
[failure] 110-110:
Delete············
[failure] 112-112:
Delete············
[failure] 114-114:
Delete············api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1)
22-24:temperaturefield resolver.
Implementation is clean, but consider whether you need defensive checks if the device is invalid.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)
110-114: Remove extraneous blank lines or whitespace.
Static analysis flags these lines as having excessive indentation or spacing.110 - + 112 -<div class="upgrade_notice"><?=_("Your browser has JavaScript disabled")?></div> +<div class="upgrade_notice"><?=_("Your browser has JavaScript disabled")?></div> 114 - +
📜 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 (14)
api/dev/states/myservers.cfg(1 hunks)api/generated-schema.graphql(6 hunks)api/src/graphql/schema/types/disks/disk.graphql(2 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.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(10 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(10 hunks)api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch(4 hunks)
✅ Files skipped from review due to trivial changes (4)
- 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/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
🚧 Files skipped from review as they are similar to previous changes (3)
- api/src/graphql/schema/types/disks/disk.graphql
- api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts
- api/generated-schema.graphql
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
api/src/graphql/generated/api/types.ts (1)
Disk(509-530)
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts
[failure] 110-110:
Delete ············
[failure] 112-112:
Delete ············
[failure] 114-114:
Delete ············
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts
[failure] 110-110:
Delete ············
[failure] 112-112:
Delete ············
[failure] 114-114:
Delete ············
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (21)
api/dev/states/myservers.cfg (2)
21-21: Review the allowedOrigins additionsThe allowedOrigins list has been updated with a new URL pattern (
https://85-121-123-122.thisisfourtyrandomcharacters012345678900.myunraid.net:8443). Given that this PR is moving disks into the service layer, verify that this change is intentional and necessary for the refactoring work. The extensive list of allowed origins should be periodically reviewed to ensure it only contains necessary entries.
24-24: LGTM: Empty UPNP statusSetting the UPNP status to an empty string is reasonable, especially if this is a development configuration file that's being reset to a clean state as part of the refactoring work.
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (3)
33-35: Removed nav-user div.
The removal of this block might affect user alert icons. If these alerts now rely on an alternative component, verify they are still surfaced in the UI.
47-57: Further jGrowl references removed.
Make sure any other modules/scripts depending on jGrowl have been updated to use your new notification pattern.
79-79: Added<uui-toaster>for notifications.
This modernizes user alerts with a more flexible toasting mechanism. Confirm styling, positioning, and event handling meet UX requirements.api/src/unraid-api/graph/resolvers/disks/disks.service.ts (2)
39-114: Partition filtering and disk mapping.
The mapping logic looks clean and explicit. Confirm that unknown fsTypes are intentionally omitted. Also, check if you want to support additional partition logic (e.g., extended vs. logical partitions).
119-128: Batch processing for disks.
UsingbatchProcessis a solid approach for concurrency. Ensure each disk object that returns fromparseDiskis valid, and confirm that any parse errors are handled or logged.api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (4)
20-23: Introduced$themeHtmlClass.
This approach centralizes theme-based styling. Good practice to keep theme handling modular. Continue verifying that the appended classes do not conflict with existing styles.
36-36: Applied<?= $themeHtmlClass ?>to<html>tag.
This helps unify the theming assignment at the root. Double-check the resulting layout across all themes.
861-863: Footer extraction.
Requiring a dedicatedFooter.phpclarifies the layout structure. Nice step for maintainability. Confirm that all dynamic variables still render properly.
1290-1290: Pause updates on visibility change.
CallingnchanFocusStop()is consistent with optimizing background resource usage. Confirm no critical background tasks are disrupted if user switches tabs.api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
4-4: Use of Vitest import looks good.
No issues here.api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (5)
1-1: Imports from NestJS GraphQL are appropriate.
No concerns here.
5-6: Service import is properly updated.
Renaming fromDiskServicetoDisksServiceis consistent with the new usage.
8-8: Resolver decorator references'Disk'.
This change aligns the resolver name with the single-disk type. Ensure other references in the codebase are updated if needed.
10-10: Constructor injection ofDisksService.
Excellent approach for testability and separation of concerns.
12-20:disksquery method delegates to service.
Implementation is straightforward and leveragesthis.disksService.getDisks(). Looks good.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (4)
20-23: Dynamic theme class assignment is well-structured.
Appending"Theme--sidebar"for certain themes is a neat approach for conditional styling.
52-58: Consolidated stylesheet imports.
Switching tojquery.ui.css,default-*CSS, and theme-based CSS improves maintainability.
81-88: Conditional styling for header navigation blocks.
Applies background and text color based on$backgnd/$headervariables. This is clean, but be sure to test with all theme variations.
853-854: Footer inclusion.
Including the external footer file (DefaultPageLayout/Footer.php) is a nice step toward modularization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1)
22-25: New temperature resolver field.This addition properly implements the GraphQL resolver pattern for resolving the temperature field of a Disk object. Consider adding error handling in case the temperature retrieval fails.
@ResolveField('temperature') public async temperature(@Parent() disk: Disk) { - return this.disksService.getTemperature(disk.device); + try { + return this.disksService.getTemperature(disk.device); + } catch (error) { + console.error(`Failed to get temperature for disk ${disk.device}:`, error); + return null; // or a default value + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/graphql/schema/types/disks/disk.graphql(3 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/graphql/schema/types/disks/disk.graphql
🧰 Additional context used
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts
[failure] 5-5:
Replace {·type·Disk, with type·{·Disk·}·from·'@app/graphql/generated/api/types.js';⏎import·{
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts
[failure] 5-5:
Replace {·type·Disk, with type·{·Disk·}·from·'@app/graphql/generated/api/types.js';⏎import·{
⏰ 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/disks/disks.resolver.ts (3)
1-1: Import statement updated to support new functionality.The import statement has been updated to include
ParentandResolveField, which are required for the new temperature field resolver functionality.
8-10: Improved architecture with service-based approach.The resolver now:
- Uses 'Disk' (singular) instead of 'Disks' (plural) for the @resolver decorator
- Implements dependency injection for DisksService instead of using standalone functions
This is a significant architectural improvement that enhances testability and maintainability.
18-20: Service delegation for disk retrieval.The resolver now delegates disk data retrieval to the injected service, which is cleaner and follows better separation of concerns.
elibosley
force-pushed
the
chore/move-disks-to-service
branch
from
eac70d8 to
983d0f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
43-45: Expand test coverage for potential errors and edge cases.
Currently, thediskstests only validate the happy-path scenarios (empty arrays, mock objects). Consider adding:
- A rejection test scenario where
getDisksrejects or throws.- A scenario to confirm behavior if temperature is disabled (ref. from earlier feedback).
Also applies to: 46-67, 68-75
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1)
5-6:⚠️ Potential issueFix the build error caused by combined import syntax.
According to the pipeline failure logs, you must separate type imports to pass the formatting check.Apply this diff to resolve the import style error:
-import { type Disk, Resource } from '@app/graphql/generated/api/types.js'; +import type { Disk } from '@app/graphql/generated/api/types.js'; +import { Resource } from '@app/graphql/generated/api/types.js';🧰 Tools
🪛 GitHub Check: Build API
[failure] 5-5:
Replace{·type·Disk,withtype·{·Disk·}·from·'@app/graphql/generated/api/types.js';⏎import·{🪛 GitHub Check: Test API
[failure] 5-5:
Replace{·type·Disk,withtype·{·Disk·}·from·'@app/graphql/generated/api/types.js';⏎import·{🪛 GitHub Actions: CI - Main (API)
[error] 5-5: Prettier formatting check failed. Replace
{ type Disk,withtype { Disk } from '@app/graphql/generated/api/types.js'; import {.
🧹 Nitpick comments (8)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (1)
61-65: Color orb removal may reduce clarityThe removal of color-coded orbs might reduce the immediate at-a-glance awareness of alert severity levels. Consider adding a visual alternative inside the new notification system to maintain clarity.
api/src/unraid-api/graph/resolvers/disks/disks.service.ts (2)
13-37: Handle temperature with cautionReturning
nullupon error or missing temperature data is reasonable. Ensure logs or metrics (if any) capture these failures to monitor disk health consistency. Also, consider verifying thatsmartctlis present and accessible in all target environments.
83-103: Interface type mappingMapping interface strings to known enums handles a variety of device labels. NVMe mapped to PCIE is standard. Double-check for variations like "M.2" or any vendor-specific designations that might appear in system reports.
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
87-88: Add negative path tests fortemperatureresolution.
Thetemperaturetests verify normal behavior, but do not confirm how the resolver responds if the service fails or if the disk device is invalid. A test scenario that mocks a service failure or an unrecognized device can strengthen the coverage.Also applies to: 89-109, 110-119
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (1)
12-20: Optional: Add explicit return types for maintainability.
Although TypeScript can infer return types, explicitly annotating the return type ofdisks()(and possiblytemperature()) often improves readability and helps prevent subtle errors in future refactors.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (3)
52-58: Ensure consistent styling across theme variants.
Adding multiple new CSS references can be beneficial, but confirm these files don’t inadvertently override user-defined themes or introduce performance overhead on slower connections. Consider a consolidated build pipeline if loading too many styles becomes an issue.
122-123: Validate required libraries for Shadowbox and context.
The calls toShadowbox.init()andcontext.init()assume these libraries are fully loaded. If these references are optional or reliant on external scripts, ensure those scripts are guaranteed to be present in all relevant environments.
1275-1277: Double-check reliability of pause/resume logic during visibility changes.
Pausing live updates on blur can be beneficial, but confirm it doesn't break essential background tasks. If some processes must continue, consider a more granular approach.
📜 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 (36)
api/dev/states/myservers.cfg(1 hunks)api/generated-schema.graphql(6 hunks)api/src/__test__/common/allowed-origins.test.ts(1 hunks)api/src/__test__/core/utils/misc/get-key-file.test.ts(2 hunks)api/src/__test__/core/utils/shares/get-shares.test.ts(1 hunks)api/src/__test__/setup.ts(1 hunks)api/src/__test__/setup/mock-fs-setup.ts(1 hunks)api/src/__test__/setup/store-reset.ts(1 hunks)api/src/__test__/store/modules/config.test.ts(2 hunks)api/src/__test__/store/modules/registration.test.ts(3 hunks)api/src/common/allowed-origins.ts(2 hunks)api/src/core/modules/get-disks.ts(0 hunks)api/src/core/modules/index.ts(0 hunks)api/src/graphql/schema/types/array/array.graphql(2 hunks)api/src/graphql/schema/types/disks/disk.graphql(3 hunks)api/src/store/actions/reset-store.ts(1 hunks)api/src/store/index.ts(1 hunks)api/src/store/modules/config.ts(2 hunks)api/src/store/modules/minigraph.ts(1 hunks)api/src/store/modules/registration.ts(1 hunks)api/src/store/root-reducer.ts(1 hunks)api/src/unraid-api/auth/cookie.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/disks/disks.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 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(10 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(10 hunks)api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch(4 hunks)api/vite.config.ts(2 hunks)
💤 Files with no reviewable changes (2)
- api/src/core/modules/index.ts
- api/src/core/modules/get-disks.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- 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/src/store/actions/reset-store.ts
- api/src/store/modules/minigraph.ts
- 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/src/unraid-api/graph/resolvers/disks/disks.module.ts
- api/vite.config.ts
- api/src/store/modules/registration.ts
- api/src/test/core/utils/shares/get-shares.test.ts
- api/src/test/setup.ts
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- api/src/store/modules/config.ts
- api/src/graphql/schema/types/disks/disk.graphql
- api/src/test/core/utils/misc/get-key-file.test.ts
- api/src/common/allowed-origins.ts
- api/src/test/setup/mock-fs-setup.ts
- api/src/unraid-api/graph/resolvers/disks/disks.service.spec.ts
- api/src/test/store/modules/registration.test.ts
- api/src/test/store/modules/config.test.ts
- api/src/test/common/allowed-origins.test.ts
- api/src/test/setup/store-reset.ts
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php
- api/src/unraid-api/auth/cookie.service.spec.ts
- api/src/graphql/schema/types/array/array.graphql
- api/dev/states/myservers.cfg
- api/generated-schema.graphql
🧰 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-04-08T05:25:22.477Z
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-04-08T05:25:22.477Z
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-04-08T05:25:22.477Z
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-04-08T05:25:22.477Z
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-04-08T05:25:22.477Z
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-04-08T05:25:22.477Z
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-04-08T05:25:22.477Z
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-04-08T05:25:22.477Z
Learning: Notifications are implemented on a separate page rather than in the DefaultPageLayout.php file.
🧬 Code Definitions (3)
api/src/store/root-reducer.ts (5)
api/src/store/modules/config.ts (1)
configReducer(348-348)api/src/store/modules/dynamic-remote-access.ts (1)
dynamicRemoteAccessReducer(73-73)api/src/store/modules/minigraph.ts (1)
mothershipReducer(91-91)api/src/store/modules/registration.ts (1)
registrationReducer(69-69)api/src/store/actions/reset-store.ts (1)
resetStore(7-7)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
api/src/graphql/generated/api/types.ts (1)
Disk(509-530)
api/src/store/index.ts (1)
api/src/store/root-reducer.ts (1)
rootReducer(35-43)
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts
[failure] 5-5:
Replace {·type·Disk, with type·{·Disk·}·from·'@app/graphql/generated/api/types.js';⏎import·{
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts
[failure] 5-5:
Replace {·type·Disk, with type·{·Disk·}·from·'@app/graphql/generated/api/types.js';⏎import·{
🪛 GitHub Actions: CI - Main (API)
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts
[error] 5-5: Prettier formatting check failed. Replace { type Disk, with type { Disk } from '@app/graphql/generated/api/types.js'; import {.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (4)
11-21: Remove jGrowl references completely?The eliminated jGrowl block aligns with the project's shift to a new notification system, as noted in the retrieved learnings. This is consistent with design decisions to handle notifications on a separate page. No concerns here.
33-35: Nav bell element removedThe removal of the bell element might impact user awareness of new notifications. Ensure that visual cues or equivalents are provided via the new
<uui-toaster>or other UI components so users remain informed about alerts.
47-57: Second jGrowl removal blockRemoving the jGrowl logic here is also consistent with the new toaster-based notification approach. Confirm that the new logic properly handles archived notifications if needed.
79-80: Integration ofAdding a
<uui-toaster>with customizable position is a great replacement for legacy notifications. If the user sets a custom preference for toaster positioning, confirm that user settings are properly mapped to thepositionattribute.api/src/store/root-reducer.ts (3)
1-2: Structured Redux importsUsing named imports from
@reduxjs/toolkitand grouping them at the top helps maintain clarity. This is good practice.
19-30: Combine reducers approachCombining multiple reducers into
appReduceris standard Redux methodology. The naming is intuitive and well-structured. No issues spotted.
35-43: Handling reset actionResetting state by passing
undefinedis an effective approach for clearing all slices. Ensure that the relevant slices handle the initial state properly to avoid undesired side effects.api/src/unraid-api/graph/resolvers/disks/disks.service.ts (2)
39-81: Partition filtering logicThe mapping from lowercased fsTypes to their enum values reduces confusion. Returning only known partitions is sensible. Be mindful that some edge cases (e.g., an unexpected fsType) might inadvertently get filtered out if it's actually valid but not listed. Confirm that’s intentional.
119-128: Batch disk parsingThe
batchProcessusage is a straightforward concurrency pattern. Confirm that partial failures (like one disk parse error) don’t prevent the collection of all other disk data.api/src/unraid-api/graph/resolvers/disks/disks.resolver.spec.ts (1)
11-15: Good approach to mocking the service.
DefiningmockDisksServicehere ensures that all test scenarios remain isolated and predictable. Also, resetting the mocks before each test is a recommended practice for maintaining test clarity.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (2)
20-23: Implementation of$themeHtmlClassis straightforward.
Appending"Theme--sidebar"if$themes2is a clear approach to controlling layout. The updated<html>tag properly reflects the dynamic class attribute.Also applies to: 36-36
80-88: Check user-supplied background and header colors.
Overwriting background colors or text colors in lines 81-88 might conflict with certain user-defined themes. If user customization is allowed, add a fallback mechanism to avoid an unexpected style override.Also applies to: 90-90
api/src/store/index.ts (1)
4-4: Root reducer consolidation appears sound.
This simplifies reducer management. Remember to ensurerootReducermerges all relevant slices. Also, if you rely on lazy-loaded reducers elsewhere, confirm no references have been inadvertently broken.Also applies to: 7-7
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
DisksModulefor better organization of disk-related functionalities.