chore: add a prefix scalar instead of prefix plugin #1361
Conversation
WalkthroughThis update introduces a new custom GraphQL scalar, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLServer
participant PrefixedIDScalar
participant Resolver/Service
Client->>GraphQLServer: Query/Mutation with ID (may include prefix)
GraphQLServer->>PrefixedIDScalar: parseValue(input ID)
PrefixedIDScalar-->>GraphQLServer: ID stripped of prefix
GraphQLServer->>Resolver/Service: Process request with raw ID
Resolver/Service-->>GraphQLServer: Return data with raw ID
GraphQLServer->>PrefixedIDScalar: serialize(raw ID)
PrefixedIDScalar-->>GraphQLServer: ID with server prefix added
GraphQLServer-->>Client: Response with prefixed ID
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 (
|
elibosley
force-pushed
the
chore/server-prefix-scalar
branch
from
7661778 to
591f627
Compare
elibosley
changed the title
elibosley
force-pushed
the
chore/server-prefix-scalar
branch
from
5df7b02 to
0a1d3b5
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: 2
🧹 Nitpick comments (4)
api/src/unraid-api/graph/resolvers/connect/connect.model.ts (1)
1-1: Consider removing unnecessary ID import.The import of
IDfrom@nestjs/graphqlis no longer needed since the classes now extend theNodebase class, which provides the ID field implementation. It can be safely removed to clean up the imports.-import { Field, ID, InputType, Int, ObjectType, registerEnumType } from '@nestjs/graphql'; +import { Field, InputType, Int, ObjectType, registerEnumType } from '@nestjs/graphql';api/src/unraid-api/graph/resolvers/docker/docker.model.ts (1)
1-1: Consider removing unnecessary ID import.The import of
IDfrom@nestjs/graphqlis no longer needed as the classes now extend theNodebase class, which provides the ID field implementation.-import { Field, ID, Int, ObjectType, registerEnumType } from '@nestjs/graphql'; +import { Field, Int, ObjectType, registerEnumType } from '@nestjs/graphql';api/src/unraid-api/graph/scalars/graphql-type-prefixed-id.ts (2)
63-70: Consider cleaning up commented codeThe commented-out code for prefix validation could be removed since it's not being used, or implemented if the validation is important.
// Simple check to avoid double-prefixing if somehow already prefixed // This might happen if data is fetched internally already prefixed if (value.includes(':')) { - // Optionally log or verify if the prefix matches the current serverId - // const serverId = this.serverIdentifierService.getId(); - // if (!value.startsWith(`${serverId}:`)) { - // console.warn(`PrefixedID serialize: Value '${value}' already has a different prefix.`); - // } return value; }
87-88: Consider logging prefix warnings in productionThe code suppresses a potentially helpful debug message when a value doesn't contain the expected prefix format. For debugging in production, consider enabling this log or adding it to a debug log system.
// If it doesn't have the prefix format, assume it's an internal ID already. -// console.debug(`PrefixedID parseValue: Value '${value}' does not contain expected prefix.`); +console.debug(`PrefixedID parseValue: Value '${value}' does not contain expected prefix.`); return value;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
api/generated-schema.graphql(39 hunks)api/src/graphql/schema/utils.ts(1 hunks)api/src/unraid-api/auth/api-key.service.spec.ts(1 hunks)api/src/unraid-api/graph/graph.module.ts(3 hunks)api/src/unraid-api/graph/id-prefix-plugin.ts(0 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts(4 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/array/array.model.ts(5 hunks)api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts(4 hunks)api/src/unraid-api/graph/resolvers/base.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/config/config.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/connect/connect-settings.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/connect/connect.model.ts(4 hunks)api/src/unraid-api/graph/resolvers/disks/disks.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.model.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/flash/flash.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts(0 hunks)api/src/unraid-api/graph/resolvers/info/info.model.ts(16 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.model.ts(3 hunks)api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts(4 hunks)api/src/unraid-api/graph/resolvers/registration/registration.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/servers/server.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/vars/vars.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/vms/vms.model.ts(3 hunks)api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts(8 hunks)api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/vms/vms.service.ts(1 hunks)api/src/unraid-api/graph/scalars/graphql-type-prefixed-id.ts(1 hunks)api/src/unraid-api/graph/services/service.model.ts(1 hunks)api/src/unraid-api/graph/user/user.model.ts(1 hunks)
💤 Files with no reviewable changes (2)
- api/src/unraid-api/graph/resolvers/flash/flash.resolver.ts
- api/src/unraid-api/graph/id-prefix-plugin.ts
🧰 Additional context used
🧬 Code Graph Analysis (8)
api/src/unraid-api/graph/resolvers/vars/vars.model.ts (1)
web/composables/gql/graphql.ts (1)
Node(932-934)
api/src/unraid-api/graph/resolvers/servers/server.model.ts (6)
api/src/unraid-api/graph/resolvers/registration/registration.model.ts (2)
ObjectType(79-86)ObjectType(88-104)api/src/unraid-api/graph/resolvers/array/array.model.ts (5)
ObjectType(9-19)ObjectType(21-28)ObjectType(30-128)ObjectType(130-151)ObjectType(239-287)api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts (3)
ObjectType(18-29)ObjectType(31-58)ObjectType(60-65)api/src/unraid-api/graph/resolvers/config/config.model.ts (1)
ObjectType(5-14)api/src/unraid-api/graph/resolvers/flash/flash.model.ts (1)
ObjectType(5-17)web/composables/gql/graphql.ts (1)
Node(932-934)
api/src/unraid-api/graph/services/service.model.ts (1)
web/composables/gql/graphql.ts (1)
Node(932-934)
api/src/unraid-api/graph/resolvers/connect/connect.model.ts (1)
web/composables/gql/graphql.ts (4)
ConnectSettings(334-344)Node(932-934)Connect(324-332)DynamicRemoteAccessStatus(590-598)
api/src/unraid-api/graph/resolvers/registration/registration.model.ts (1)
web/composables/gql/graphql.ts (2)
Node(932-934)Registration(1128-1136)
api/src/unraid-api/graph/resolvers/notifications/notifications.model.ts (1)
web/composables/gql/graphql.ts (2)
Node(932-934)Notifications(991-997)
api/src/unraid-api/graph/resolvers/array/array.model.ts (1)
web/composables/gql/graphql.ts (4)
ArrayDisk(102-145)Node(932-934)UnraidArray(1350-1369)Share(1265-1298)
api/src/unraid-api/graph/scalars/graphql-type-prefixed-id.ts (1)
api/src/core/utils/server-identifier.ts (1)
getServerIdentifier(6-9)
⏰ 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 (74)
api/src/unraid-api/graph/resolvers/vms/vms.service.spec.ts (3)
264-264: Update to validateidproperty existenceThis change correctly verifies the existence of the new
idproperty that replaced the previousuuidfield, ensuring test compliance with the PrefixedID scalar implementation.
267-267: Updated VM start function to useidpropertyProperly updated to use
testVm!.idinstead ofuuidwhen callingservice.startVm(), maintaining consistency with the schema changes implementing the Node interface.
285-285: Updated VM stop function to useidpropertySuccessfully switched from
uuidtoidwhen callingservice.stopVm(), completing the implementation of the PrefixedID scalar pattern throughout VM operations.api/src/unraid-api/graph/resolvers/registration/registration.resolver.ts (1)
36-36: Renamed property fromguidtoidChanged the property name in the registration object from
guidtoidwhile retaining the same value source fromemhttp.var.regGuid. This aligns with the Node interface implementation where entities use a standardidfield with the PrefixedID scalar.api/src/unraid-api/graph/resolvers/vms/vms.service.ts (1)
363-363: Property name changed fromuuidtoidUpdated the domain object mapping to use
idinstead ofuuidas the property name, while still using the UUID string value. This properly aligns the service response with the GraphQL schema changes adopting the PrefixedID scalar.api/src/graphql/schema/utils.ts (2)
86-86: Set explicit server IDAdded explicit
'local'identifier for the server object'sidproperty, ensuring compatibility with the new PrefixedID scalar implementation that requires server identifiers for uniqueness across multiple servers.
88-88: Set explicit owner IDAdded explicit
'local'identifier for the owner object'sidproperty, maintaining consistency with the server object and completing the migration to the PrefixedID scalar pattern.api/src/unraid-api/graph/resolvers/vars/vars.model.ts (1)
35-35: Good refactoring to extend Node base classThe change from implementing the Node interface to extending the Node base class is a good approach. This eliminates the need for redundant
idfield declarations while maintaining the GraphQL interface implementation (through the@ObjectTypedecorator).api/src/unraid-api/graph/resolvers/connect/connect-settings.resolver.ts (2)
27-27: Import of PrefixedID scalarGood addition of the PrefixedID scalar import to replace the standard ID type.
35-35: Updated to use PrefixedID scalarCorrectly updated the return type from ID to PrefixedID, which aligns with the removal of the idPrefixPlugin in favor of the scalar approach.
api/src/unraid-api/auth/api-key.service.spec.ts (1)
523-523: More explicit test assertionsThe test now properly specifies that createdAt should be a string type. This increases test precision and makes expectations clearer.
Also applies to: 529-530
api/src/unraid-api/graph/services/service.model.ts (1)
14-14: Good refactoring to extend Node base classSimilar to the Vars model, this change appropriately leverages inheritance from the Node base class while maintaining the GraphQL interface implementation through the decorator. This reduces code duplication and standardizes ID handling across models.
api/src/unraid-api/graph/resolvers/base.model.ts (2)
1-5: Good refactoring of imports to support the new PrefixedID scalarThe removal of the ID import and addition of PrefixedID along with validation decorators shows good attention to detail. This change forms the foundation for the entire migration.
47-50: Excellent addition of validation decoratorsThe validation decorators
@IsString()and@IsNotEmpty()ensure that theidfield is a non-empty string, which aligns perfectly with the expected behavior of thePrefixedIDscalar. This helps prevent runtime errors by validating the data at the model level.api/src/unraid-api/graph/resolvers/config/config.model.ts (2)
1-3: Clean import refactoringRemoval of the ID import and reliance on the Node class import simplifies the code.
8-8: Good use of inheritanceChanging from implements to extends is a solid refactoring that reduces code duplication by inheriting the
idfield with its newPrefixedIDtype and validation decorators.api/src/unraid-api/graph/resolvers/flash/flash.model.ts (2)
1-3: Clean import refactoringRemoval of the ID import and reliance on the Node class import simplifies the code.
8-11:❓ Verification inconclusive
Consider evaluating the relationship between id and guid
While the change to extend Node is good, note that Flash now has both an inherited
idfield and an explicitguidfield. Consider whether there's redundancy between these fields or if they serve distinct purposes.
🏁 Script executed:
#!/bin/bash # Find where Flash.guid is used in the codebase to understand its purpose rg -A 3 "Flash.*guid" --type tsLength of output: 33
Review Comment Revision: Evaluate the Relationship Between
idandguidin FlashThe enhancement to extend
Nodeis a positive change. However, sinceFlashnow inherits anidfield while also defining aguidfield explicitly, it's unclear whether these fields serve distinct purposes or if there’s redundancy. An automated search for usages ofFlash.guiddid not return relevant results, which suggests that more manual investigation may be needed.Please verify:
- Whether the inherited
idfield (fromNode) and the explicitguidare intentionally distinct—e.g., one might be used internally while the other is exposed externally.- That any unique identifier requirements for
Flashare adequately met without unnecessary duplication.- The design and documentation of the base
Nodeclass to clarify whatidrepresents compared toguid.api/src/unraid-api/graph/graph.module.ts (3)
17-18: Clear import structure for scalarsThe updated import path for GraphQLLong and the addition of PrefixedID ensure all scalars are consistently imported from the scalars directory.
44-44: Simplified plugin configurationRemoving the idPrefixPlugin is consistent with moving to a scalar-based approach for ID prefixing.
65-65: Proper scalar registrationAdding PrefixedIDScalar to the providers array ensures it's available throughout the application.
api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.ts (2)
19-19: New import for PrefixedID scalar added correctlyThe import for the custom PrefixedID scalar has been added properly, which will be used to replace the standard ID scalar.
44-47: Correctly updated id argument type to PrefixedIDThe id argument in the apiKey resolver has been properly updated to use the custom PrefixedID scalar type. This is in line with the overall change to replace the id prefix plugin with a scalar-based approach.
api/src/unraid-api/graph/resolvers/registration/registration.model.ts (2)
1-3: Import updates align with Node inheritance patternRemoved the ID import from @nestjs/graphql and added the Node import from base.model, which is appropriate given that Registration now extends Node.
88-89: Registration class now properly extends NodeThe Registration class has been updated to extend the Node base class and implement the Node interface. This change standardizes the ID handling across entities and removes the need for explicit id field declarations.
api/src/unraid-api/graph/resolvers/disks/disks.resolver.ts (2)
11-11: Added PrefixedID scalar importThe import for the custom PrefixedID scalar has been properly added to support the updated argument type in the resolver method.
33-33: Disk resolver now uses PrefixedID scalar for id argumentThe id argument in the disk query resolver has been correctly updated to use the PrefixedID scalar, which ensures consistent ID handling throughout the API.
api/src/unraid-api/graph/resolvers/servers/server.model.ts (3)
3-3: Added Node import to support inheritanceThe import for Node from base.model.js has been added correctly to support the class inheritance changes.
5-6: ProfileModel now extends NodeProfileModel has been updated to implement the Node interface and extend the Node base class, ensuring consistent ID handling.
27-28: Server class now extends NodeThe Server class has been updated to implement the Node interface and extend the Node base class, aligning with the standardized approach to ID handling across the application.
api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts (1)
27-28: LGTM: Types properly updated to use PrefixedIDThe mutation arguments now correctly use the PrefixedID scalar type while maintaining string as the TypeScript type.
Also applies to: 37-38
api/src/unraid-api/graph/user/user.model.ts (1)
6-7: LGTM: Properly extends and implements NodeThe UserAccount class correctly extends the Node base class while also implementing the Node interface, which aligns with the pattern of consolidating id fields in the base class.
api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts (1)
116-118: LGTM: Input fields properly updated to PrefixedIDThe apiKeyId fields in the input types correctly use the PrefixedID scalar.
Also applies to: 127-129
api/src/unraid-api/graph/resolvers/connect/connect.model.ts (4)
20-20: Good job adding the PrefixedID import.The import of the custom scalar is necessary to support the change to server-prefixed IDs.
330-342: Good refactoring to extend Node base class.Changing from implementing the
Nodeinterface to extending theNodebase class promotes code reuse and reduces duplication. This approach centralizes the ID field implementation in one place.
347-356: Well-structured inheritance from Node base class.The Connect class now properly extends Node while maintaining all its specific fields.
361-364: Consistent application of the Node base class.The Network class has been updated to follow the same pattern as other classes, maintaining consistency across the codebase.
api/src/unraid-api/graph/resolvers/docker/docker.model.ts (3)
74-75: Good refactoring to extend the Node base class.Changing DockerContainer to extend the Node base class while still implementing the Node interface through the decorator is a good approach that promotes code reuse and maintains type safety.
119-120: Consistent application of the Node base class pattern.The DockerNetwork class follows the same pattern, ensuring consistency across the codebase.
167-167: Well-structured inheritance from Node.The Docker class properly extends the Node base class, aligning with the broader refactoring in the codebase.
api/src/unraid-api/graph/resolvers/notifications/notifications.model.ts (3)
1-6: Clean import adjustments.Good job removing the unused ID import and adding the Node import. This keeps the imports concise and relevant to what's actually used in the file.
114-117: Well-structured refactoring and cleanup.Excellent work on both extending the Node base class and removing the duplicate @field decorator for the title property. This improves code cleanliness and follows the project's architectural patterns.
158-159: Consistent application of Node inheritance.The Notifications class now properly extends Node and implements the Node interface, maintaining consistency with other model classes in the codebase.
api/src/unraid-api/graph/resolvers/notifications/notifications.resolver.ts (6)
21-21: Appropriate import of PrefixedID scalar.Adding the import for the custom scalar type is necessary to support the argument type changes in the resolver methods.
70-71: Good scalar type update for ID argument.Changing from String to PrefixedID scalar ensures IDs are properly prefixed with server identifiers. Note that the parameter type in the function signature is still string, which is correct since the scalar conversion happens at the GraphQL layer.
88-90: Consistent scalar usage for ID argument.Using PrefixedID scalar consistently across all ID-related arguments maintains a uniform approach to ID handling.
96-98: Proper handling of ID array type.Correctly updated the array of IDs to use the PrefixedID scalar. This ensures consistent handling of ID collections.
114-116: Consistent scalar usage for ID argument.Using PrefixedID scalar for the unreadNotification method maintains consistency with other mutation methods.
122-124: Proper handling of ID array type.Correctly updated the array of IDs to use the PrefixedID scalar in the unarchiveNotifications method.
api/src/unraid-api/graph/resolvers/vms/vms.model.ts (3)
1-6: Proper imports for the new Node and PrefixedID approach.The imports correctly bring in the necessary dependencies for implementing the Node interface and using the new PrefixedID scalar type, along with class-validator decorators for input validation.
25-30: Good implementation of Node interface with proper validation.The VmDomain class now correctly implements the Node interface, and the id field is properly typed with PrefixedID. The addition of validation decorators (@IsString and @isnotempty) ensures that the ID will always be properly validated.
39-40: Appropriate extension of Node base class.The Vms class now properly extends the Node base class, eliminating redundant id field declaration and ensuring consistent ID handling.
api/src/unraid-api/graph/resolvers/array/array.mutations.resolver.ts (4)
18-18: Correctly importing the PrefixedID scalar.The PrefixedID import is appropriately added to support the scalar usage in resolver arguments.
66-66: Properly typed id parameter in mountArrayDisk.The id parameter is now correctly typed with PrefixedID, ensuring consistent ID handling across the API.
86-88: Properly typed id parameter in unmountArrayDisk.The id parameter is now correctly typed with PrefixedID, maintaining type consistency throughout the API.
108-110: Properly typed id parameter in clearArrayDiskStatistics.The id parameter is now correctly typed with PrefixedID, completing the consistent ID handling across all array-related operations.
api/src/unraid-api/graph/resolvers/info/info.model.ts (3)
14-14: Correctly importing the PrefixedID scalar.The PrefixedID import is appropriately added to support the new scalar usage in the info models.
35-36: Consistent implementation of Node pattern across multiple classes.All these information-related model classes now properly implement the Node interface and extend the Node base class, which eliminates redundant id field declarations and ensures consistent ID handling throughout the API. This refactoring significantly reduces code duplication.
Also applies to: 44-45, 62-63, 116-117, 137-138, 176-177, 203-204, 209-210, 221-222, 236-237, 299-300, 335-336, 371-372, 410-411, 431-432, 512-513
529-529: Updated machineId field to use PrefixedID scalar.The machineId field type has been correctly updated from ID to PrefixedID to maintain consistency with the new approach to ID handling.
api/src/unraid-api/graph/resolvers/array/array.model.ts (5)
6-7: Updated imports for GraphQLLong and added PrefixedID.The import for GraphQLLong has been updated to use the new path, and PrefixedID has been properly added for use in the array models.
33-33: ArrayDisk now extends Node base class.ArrayDisk class now properly extends the Node base class, eliminating the need for a redundant id field declaration.
133-133: UnraidArray now extends Node base class.UnraidArray class now properly extends the Node base class, maintaining consistency with the new pattern.
155-155: Updated id field in ArrayDiskInput to use PrefixedID.The id field in the ArrayDiskInput class is now correctly typed with PrefixedID, ensuring type consistency between inputs and models.
242-242: Share now extends Node base class.Share class now properly extends the Node base class, completing the consistent implementation across all array-related models.
api/src/unraid-api/graph/scalars/graphql-type-prefixed-id.ts (1)
1-107: Well-implemented custom scalar for ID prefixingThe PrefixedID scalar is a good approach to handling server-specific ID prefixing. The implementation properly handles both input (stripping prefix when present) and output (always adding prefix) scenarios.
A few observations:
- Good detailed documentation on functionality in the description
- Appropriate error handling for non-string values
- Clean implementation of the required scalar methods
api/src/unraid-api/graph/resolvers/vms/vms.mutations.resolver.ts (2)
11-11: Import of new PrefixedID scalarCorrectly added import for the new PrefixedID scalar type.
26-26: Consistent type annotations for ID argumentsThe resolver methods have been updated consistently to use the PrefixedID scalar type for all ID arguments, ensuring proper handling of server-prefixed IDs throughout the VM mutation operations.
Also applies to: 36-36, 46-46, 56-56, 66-66, 76-76, 86-86
api/generated-schema.graphql (7)
77-77: Consistent type changes across Node implementationsAll types implementing the Node interface have been updated to use PrefixedID instead of ID, ensuring consistent handling of server-prefixed IDs throughout the schema.
Also applies to: 145-145, 183-183, 219-219, 350-350, 368-368, 378-378, 382-384, 389-391
826-832: Updated array mutation arguments to use PrefixedIDArray-related mutation arguments have been properly updated to use the PrefixedID scalar.
855-859: Updated Docker mutation arguments to use PrefixedIDDocker-related mutation arguments have been properly updated to use the PrefixedID scalar.
863-882: Updated VM mutation arguments to use PrefixedIDVM-related mutation arguments have been properly updated to use the PrefixedID scalar.
1390-1391: Addition of PrefixedID scalar to schemaThe PrefixedID scalar has been properly defined with comprehensive documentation explaining its behavior for both input and output operations.
1484-1484: Updated API key input types to use PrefixedIDInput types for API key operations have been properly updated to use the PrefixedID scalar.
Also applies to: 1489-1489
1234-1236:❓ Verification inconclusive
Changed
useCachetoskipCachewith inverted defaultThe Docker type's container and network query fields have been updated to use
skipCachewith a default offalseinstead ofuseCachewith a default oftrue. This change inverts the logic, so ensure any client code is updated accordingly.Verify that client code has been updated to handle this change:
🏁 Script executed:
#!/bin/bash # Search for any client code using the old useCache parameter rg -A 2 -B 2 "useCache.*Docker" --glob "*.{ts,js,tsx,jsx}"Length of output: 60
Client Code Verification Required: Confirm
skipCacheUpdate in Docker FieldsThe GraphQL Docker type now uses
skipCachewith a default offalse(inverting the previous logic fromuseCachewith defaulttrue) for bothcontainersandnetworks. The initial search for client references to the olduseCacheparameter returned no matches. However, please manually verify that all client code has updated accordingly.
- Modified file:
api/generated-schema.graphql(Lines: 1234-1236)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/composables/gql/graphql.ts (2)
25-26: Consider validating port range
Introducing thePortscalar enhances type safety for port numbers. If possible, add server-side validation to reject out-of-range or privileged ports (below 1024) if your use case disallows them.
312-317: NewAuthActionVerbenum
This addition clarifies the allowed CRUD actions in your auth system. If relevant, consider including additional verbs likeLISTorEXECUTE.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
web/codegen.ts(1 hunks)web/components/Notifications/graphql/notification.query.ts(2 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(53 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/codegen.ts
- web/composables/gql/gql.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (32)
web/components/Notifications/graphql/notification.query.ts (2)
38-38: Prefixed ID mutation argument
Changing the mutation argument from a generic ID or string toPrefixedID!is consistent with the new globally unique ID approach. Ensure all calls that pass theidvariable are updated accordingly.
62-62: Confirm external references for the new scalar
UsingPrefixedID!fordeleteNotificationaligns with the refactor. Double-check that related client code and any test cases now supply the new scalar format to avoid runtime errors.web/composables/gql/graphql.ts (30)
27-74: Excellent documentation forPrefixedID
This thorough comment block clearly explains how the prefix is handled on input and output. Good job ensuring it's self-explanatory for future maintainers.
87-93: Typed URL fields
Switching from simple strings toURLscalars (foripv4andipv6) provides better clarity and stricter type checks. This helps catch invalid URLs earlier.
100-100: Consistent use ofPrefixedID
EnsuringapiKeyIdis aPrefixedIDaligns with the global ID scheme. Just confirm call sites pass the prefixed form to prevent mismatches.
109-113:ApiKeytype update
MovingidtoScalars['PrefixedID']['output']is consistent with the Node interface changes. This helps maintain a uniform ID strategy across the codebase.
125-129:ApiKeyWithSecretID correction
RefactoringidtoPrefixedIDis a good continuation of the unified ID approach. No issues noted here.
178-178:ArrayDiskID
The disk'sidfield now usesPrefixedID. This is consistent with the broader refactor.
217-217:ArrayDiskInputID
Updating the field toPrefixedID['input']keeps the schema consistent. Make sure any user inputs match the prefixed format.
264-264:ClearArrayDiskStatisticsArgupdate
SwitchingidtoPrefixedIDfor clearing disk statistics maintains the standard ID approach.
269-269:MountArrayDiskArgsID
EnsuringidisPrefixedID['input']helps unify how array disk operations handle IDs.
284-284:UnmountArrayDiskArgsID
Again, consistent with the overallPrefixedIDrefactor for disk-related mutations.
319-324: NewAuthPossessionenum
DefiningANY,OWN, andOWN_ANYclarifies resource ownership constraints. This is a useful approach to handle role-based access.
329-329:BaseboardID
Swapping toPrefixedIDensures this node’s identifier is uniform with others.
351-351:CaseID
Refactoredidfield also adheres to the new prefix logic.
375-375:ConfigID
Good move, no issues. Matches the node-based ID scheme.
392-392:ConnectID
Continued consistency for theConnectnode.
454-455: Use of thePortscalar
ApplyingPortforprivatePortandpublicPortensures type safety around container ports. Confirm server-side checks to guarantee valid port mappings.
495-495:DiskID
No concerns; follows thePrefixedIDstandard.
611-611:DockerContainerID
Leveraging the new scalar here is a positive step for consistent container identification.
635-635:DockerMutationsStartArgs
UsingPrefixedIDfor starting a container is aligned with the unifying ID approach.
640-640:DockerMutationsStopArgs
Similarly consistent with the new ID approach here as well.
652-652:DockerNetworkID
No issues; matches the existing pattern.
697-697:GpuID
Maintaining the samePrefixedIDpattern.
712-712:InfoID
Adopting the new scalar type for the system info node.
714-714:machineIdset toPrefixedID
Enabling machine-level prefixing for the internal machine ID fosters consistent referencing.
739-739:InfoCpuID
Transitioning the CPU info ID toPrefixedIDis in line with the rest.
759-759:InfoMemoryID
Follows the same prefixed pattern to unify memory info references.
799-804:MemoryLayoutrefactor
DeclaringidasPrefixedIDconsolidates the node-based architecture.
682-682:EnableDynamicRemoteAccessInput
Replacing a simpler scalar withAccessUrlInputfor theurlfield makes the incoming data more structured and explicit.
688-688:FlashID
Keeping flash device identifiers aligned with the new scalar.
1684-1688:Vmstype changes
Addingdomain?: Maybe<Array<VmDomain>>and switchingidtoPrefixedIDunify the VM schema with the rest of the codebase.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
elibosley
force-pushed
the
chore/server-prefix-scalar
branch
from
3080c63 to
3fd7a71
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
web/composables/gql/graphql.ts (3)
87-92:AccessUrlInputlacks URL validation
ipv4/ipv6are typed asURL, but nothing prevents a caller from sendingftp://…,javascript:…, or even''(empty string).
If the server relies on these being HTTP(S) you should:- ipv4?: InputMaybe<Scalars['URL']['input']>; + /** Must start with http:// or https:// */ + ipv4?: InputMaybe<Scalars['URL']['input']>;…and mirror that rule in both the server scalar and front-end helpers.
1067-1085: ReturnJSON→ define a real type
ParityCheckMutations.*all returnJSON. This removes type-safety and forces callers to cast. Unless the shape is genuinely unstable, declare a dedicated payload, e.g.type ParityCheckResult { ok: Boolean! message: String errors: [String!] }and swap the fields to that. It costs almost nothing and pays dividends in autocompletion.
1641-1657: VM mutation grouping is great – small DX polishNice consolidation under
vm. Two low-effort ergonomics wins:
- Add a union return type (
VmActionResult), mirroring the parity-check comment above.- Consider a single
action(id:, verb:)mutation instead of eight nearly identical fields; fewer documents & codegen artifacts.Not blocking, but worth a think.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/generated-schema.graphql(39 hunks)web/composables/gql/graphql.ts(52 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/generated-schema.graphql
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Deploy Plugin / Build and Deploy Plugin
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
web/composables/gql/graphql.ts (1)
949-952: Be prepared for widespreadidtype changesEvery
Nodenow exposesPrefixedID. Generated documents were updated, but any handwritten GraphQL strings (tests, curl snippets, monitoring dashboards) will break if they still pass a bareID.Action items
• grep the repo for"$id"orid: IDto catch stragglers.
• run E2E tests with a mixed-server environment to ensure prefix handling works.
Summary by CodeRabbit
New Features
PrefixedIDscalar type for all GraphQL IDs, ensuring unique identifiers across multiple servers by prefixing IDs with a server identifier.PrefixedIDscalar in the API schema.Portand input typeAccessUrlInputfor improved type safety.Refactor
IDorStringwithPrefixedIDfor IDs in queries, mutations, and models.Nodebase class across models, removing redundantiddeclarations.PrefixedID.PrefixedIDto TypeScriptstring.Bug Fixes
Chores