feat: add management page for API keys #1408
Conversation
WalkthroughThis update introduces comprehensive support for updating API keys, including new GraphQL mutations, input types, service methods, and UI integration. It adds a centralized store for API key modal state, refactors API key management components, and removes deprecated remote access and allowed origins management. Several new dialog UI components and schema changes are included. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as APIKeyManager.vue
participant Store as useApiKeyStore
participant GQL as GraphQL API
participant Service as ApiKeyService
UI->>Store: Show modal (edit/create)
UI->>GQL: Query possible roles/permissions
UI->>Store: Set editingKey (if editing)
UI->>GQL: Mutation (create/update API key)
GQL->>Service: create/update({id, name, ...})
Service-->>GQL: Returns updated/created key
GQL-->>UI: Return ApiKeyWithSecret
UI->>Store: Set createdKey, hide modal
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
web/components/ConnectSettings/ConnectSettings.ce.vue (1)
105-114: Consider internationalization for the button text.The "Create API Key" text is hardcoded, but I notice there are commented-out i18n imports at the top of the file. Consider using internationalization for consistency with the rest of the application.
- <BrandButton - variant="outline-primary" - padding="lean" - size="12px" - class="leading-normal" - @click="modalStore.apiKeyModalShow" - > - Create API Key - </BrandButton> + <BrandButton + variant="outline-primary" + padding="lean" + size="12px" + class="leading-normal" + @click="modalStore.apiKeyModalShow" + > + {{ t('createApiKey') }} + </BrandButton>web/components/ApiKey/PermissionCounter.vue (2)
15-25: Optimize the permission counting logic.The current counting approach uses nested
reduceandfilterwhich has O(nmk) complexity. Consider optimizing for better performance:const actionCounts = computed(() => { - const actions = possibleActions.value; const counts: Record<string, number> = {}; - for (const action of actions) { - counts[action] = props.permissions.reduce( - (sum, perm) => sum + perm.actions.filter((a) => a === action).length, - 0 - ); - } + + // Initialize counts + possibleActions.value.forEach(action => { + counts[action] = 0; + }); + + // Single pass through permissions + props.permissions.forEach(perm => { + perm.actions.forEach(action => { + if (counts.hasOwnProperty(action)) { + counts[action]++; + } + }); + }); + return counts; });
30-34: Add spacing between action counts.The action counts are rendered without spacing, making them hard to read when multiple actions are present.
- (<span v-for="action in possibleActions" :key="action" class="text-xs text-muted-foreground"> + (<span v-for="(action, index) in possibleActions" :key="action" class="text-xs text-muted-foreground"> + <template v-if="index > 0">, </template> {{ action }}: {{ actionCounts[action] || 0 }} </span>)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts (1)
37-41: Consider extracting validation logic to reduce duplication.The validation logic is correct and essential for security. However, this exact validation is duplicated in the
updatemethod (lines 95-99). Consider extracting this into a helper function to follow the DRY principle.+function validateRolesAndPermissions(roles?: any[], permissions?: any[]) { + const hasRoles = Array.isArray(roles) && roles.length > 0; + const hasPermissions = Array.isArray(permissions) && permissions.length > 0; + if (!hasRoles && !hasPermissions) { + throw new Error('At least one role or one permission is required.'); + } +} async create(@Args('input') unvalidatedInput: CreateApiKeyInput): Promise<ApiKeyWithSecret> { const input = await validateObject(CreateApiKeyInput, unvalidatedInput); - const hasRoles = Array.isArray(input.roles) && input.roles.length > 0; - const hasPermissions = Array.isArray(input.permissions) && input.permissions.length > 0; - if (!hasRoles && !hasPermissions) { - throw new Error('At least one role or one permission is required to create an API key.'); - } + validateRolesAndPermissions(input.roles, input.permissions);api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts (1)
114-118: Clever validation approach!Using a dummy property with
@AtLeastOneOffor cross-field validation is an elegant solution. The validation logic ensures API keys cannot be created without roles or permissions.Consider a more descriptive name for the dummy property:
- _atLeastOne!: any; + _atLeastOneRoleOrPermission!: any;web/components/ApiKey/ApiKeyCreate.vue (1)
46-71: Avoid creating orphaned reactive chains inside the watcherEvery time
editingKeychanges,useFragmentinstantiates new reactive proxies that are never torn down, which can accumulate watchers if the dialog is opened/closed many times.
A simple optimisation is caching the fragment once and re-using it, or callinguseFragmentoutside the watcher and updating the underlying ref instead of recreating it.
While not catastrophic here, it’s a quick win for memory hygiene.unraid-ui/src/components/common/dialog/DialogContent.vue (1)
24-31: Provide a fallback whenteleportTargetis undefinedIf
useTeleport()fails to resolve a target (e.g. the consumer forgot to register one),<DialogPortal :to="undefined">silently renders nothing, leaving the user with a blank screen.
Consider defaulting to'body'or logging a warning so the failure is explicit:-const { teleportTarget } = useTeleport(); +const { teleportTarget } = useTeleport(); + +// Fallback to document.body if no target is provided +const portalTarget = computed(() => teleportTarget.value ?? 'body');and then bind
:to="portalTarget".
This tiny guard avoids a head-scratcher during integration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/generated-schema.graphql(2 hunks)api/src/unraid-api/auth/api-key.service.ts(2 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts(3 hunks)api/src/unraid-api/graph/resolvers/validation.utils.ts(2 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/ApiKeys.page(1 hunks)unraid-ui/src/components/common/dialog/Dialog.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogClose.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogContent.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogDescription.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogFooter.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogHeader.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogScrollContent.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogTitle.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogTrigger.vue(1 hunks)unraid-ui/src/components/common/dialog/index.ts(1 hunks)unraid-ui/src/index.ts(2 hunks)web/components/ApiKey/ApiKeyCreate.vue(5 hunks)web/components/ApiKey/ApiKeyManager.vue(3 hunks)web/components/ApiKey/ApiKeyModal.vue(1 hunks)web/components/ApiKey/PermissionCounter.vue(1 hunks)web/components/ApiKey/apikey.query.ts(2 hunks)web/components/ApiKeyPage.ce.vue(1 hunks)web/components/ConnectSettings/ConnectSettings.ce.vue(3 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(5 hunks)web/nuxt.config.ts(1 hunks)web/pages/apikey.vue(1 hunks)web/pages/index.vue(1 hunks)web/store/modal.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
web/store/modal.ts (4)
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:52:57.888Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files ensures that all web components share a single Pinia store instance, which is the desired behavior. Without this initialization, each web component would have its own isolated store, breaking the intended architecture.
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:52:57.888Z
Learning: In the unraid/api project, Vue components are compiled into web components. The `setActivePinia(createPinia())` call at the module level in store files is intentional and ensures all web components share a single Pinia store instance, which is the desired behavior. This shared state approach is critical for the application's architecture to function correctly.
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:33:13.215Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files would break the build by causing all web components to share a singular Pinia store instance. Each web component needs its own Pinia store instance to function correctly.
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:33:13.215Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files would break the build by causing all web components to share a singular Pinia store instance. Each web component needs its own Pinia store instance to maintain proper isolation and encapsulation.
🧬 Code Graph Analysis (4)
api/src/unraid-api/auth/api-key.service.ts (1)
web/composables/gql/graphql.ts (3)
ApiKeyWithSecret(185-194)Permission(1153-1157)AddPermissionInput(109-112)
api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts (2)
api/src/unraid-api/graph/resolvers/validation.utils.ts (1)
AtLeastOneOf(45-63)web/composables/gql/graphql.ts (2)
UpdateApiKeyInput(1500-1506)AddPermissionInput(109-112)
web/components/ApiKey/apikey.query.ts (1)
web/composables/gql/gql.ts (1)
graphql(240-242)
web/composables/gql/gql.ts (1)
api/src/graphql/generated/client/gql.ts (1)
graphql(54-56)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (45)
api/dev/Unraid.net/myservers.cfg (1)
2-2:Details
❓ Verification inconclusive
Verify the API version bump is appropriate.
The version jump from "4.4.1" to "4.8.0" is significant. Ensure this version correctly reflects the API changes and that all dependent systems are compatible with this new version.
🏁 Script executed:
#!/bin/bash # Description: Check for references to the old API version and verify version consistency # Expected: Find any hardcoded version references that might need updating echo "Searching for references to old API version 4.4.1..." rg "4\.4\.1" --type-not binary echo "Searching for other API version references..." rg "version.*=.*[\"']4\.[0-9]+\.[0-9]+[\"']" --type-not binary echo "Checking for API version compatibility documentation..." fd -e md -e txt -e rst | xargs rg -l "version.*4\.[0-9]" 2>/dev/null || echo "No version documentation found"Length of output: 630
Internal references to version 4.4.1 not found
No hardcoded occurrences of the old version or documentation referencing any 4.x.* release were detected. The bump to 4.8.0 appears localized to
api/dev/Unraid.net/myservers.cfg.
- Please confirm that any external clients, CI/CD pipelines, deployment scripts, or downstream services that rely on version 4.4.1 are updated and compatible with 4.8.0 before merging.
web/pages/apikey.vue (1)
6-6: Clean component-based approach.The simplified template that delegates to
ApiKeyManagerfollows good architectural principles by centralizing the API key management UI in a dedicated component.web/nuxt.config.ts (1)
159-162:Details
✅ Verification successful
Verify the referenced component exists.
The custom element registration follows the correct pattern and naming convention. Ensure the referenced component
@/components/ApiKeyPage.ceexists and is properly implemented.
🏁 Script executed:
#!/bin/bash # Description: Verify the ApiKeyPage.ce component exists # Expected: Find the component file and check its basic structure echo "Checking if ApiKeyPage.ce component exists..." fd "ApiKeyPage.ce" web/components/ echo "Checking for any .ce.vue files in components directory..." fd "\.ce\.vue$" web/components/ echo "Verifying component exports..." if [ -f "web/components/ApiKeyPage.ce.vue" ]; then echo "Found ApiKeyPage.ce.vue, checking its structure..." head -20 web/components/ApiKeyPage.ce.vue else echo "ApiKeyPage.ce.vue not found - this may cause runtime errors" fiLength of output: 1513
Confirmed:
ApiKeyPage.ce.vueExists and Is Properly ImplementedThe custom element file
web/components/ApiKeyPage.ce.vuewas found and contains the expected<script>,<template>, and<style>sections wrapping theApiKeyManagercomponent. No further action is needed—configuration change is safe to merge.web/pages/index.vue (2)
124-124: Consistent integration with existing test page structure.The component usage follows the established pattern of this development page with appropriate headings and separators.
Also applies to: 152-152
12-13:Details
✅ Verification successful
Verify imported components exist.
The component imports follow the established pattern for this testing page. Ensure both
ModalsCeandConnectSettingsCecomponents exist and are properly exported.
🏁 Script executed:
#!/bin/bash # Description: Verify the imported components exist # Expected: Find both component files echo "Checking for ModalsCe component..." fd "Modals.ce" web/components/ echo "Checking for ConnectSettingsCe component..." fd "ConnectSettings.ce" web/components/ConnectSettings/ echo "Verifying component exports..." if [ -f "web/components/Modals.ce.vue" ]; then echo "Found Modals.ce.vue" head -10 web/components/Modals.ce.vue fi if [ -f "web/components/ConnectSettings/ConnectSettings.ce.vue" ]; then echo "Found ConnectSettings.ce.vue" head -10 web/components/ConnectSettings/ConnectSettings.ce.vue fiLength of output: 1358
Imported components verified
BothModalsCeandConnectSettingsCeexist inweb/components/and are properly exported.Approving these changes.
web/components/ApiKeyPage.ce.vue (1)
1-12: LGTM! Clean wrapper component.This is a well-structured, minimal wrapper component that follows Vue 3 composition API best practices. The import path, template structure, and CSS imports are all appropriate.
web/components/ConnectSettings/ConnectSettings.ce.vue (2)
7-7: Good modal store integration.The import and usage of
useModalStorefollows the established pattern in the codebase.
77-77: Modal store integration looks correct.The instantiation of
modalStoreand its usage in the button click handler follows the expected pattern.unraid-ui/src/components/common/dialog/DialogHeader.vue (1)
1-14: Well-structured dialog header component.This component follows excellent practices:
- Proper TypeScript prop definitions
- Correct use of the
cnutility for class merging- Responsive design with
sm:text-left- Clean slot-based content projection
The component structure is consistent with other UI components in the codebase.
unraid-ui/src/index.ts (2)
21-30: Clean import additions.The dialog component imports are well-organized and follow the established import pattern in the file.
178-185: Proper export structure maintained.The dialog component exports are added in the correct alphabetical location and maintain consistency with the existing export pattern.
unraid-ui/src/components/common/dialog/Dialog.vue (2)
1-8: Dialog.vue: Props and emits forwarding is solid
ThedefineProps/defineEmitssetup and use ofuseForwardPropsEmitscorrectly captures and forwards all props and event emissions fromDialogRoot.
10-14: Dialog.vue: Template binding is correct
The template cleanly bindsforwardedtoDialogRootand exposes a default slot. Looks good for flexible content projection.plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/ApiKeys.page (2)
1-4: API Keys page metadata is configured properly
TheMenu,Title,Icon, andTagfields correctly register the new “API Keys” page in the plugin menu.
6-8: API Keys page template is well-wrapped
Wrapping<unraid-api-key-manager />in<unraid-i18n-host>ensures localization support.unraid-ui/src/components/common/dialog/DialogFooter.vue (2)
1-6: DialogFooter.vue: Props definition is clear
UsingdefineProps<{ class?: HTMLAttributes['class'] }>()together with thecnutility cleanly merges base and user-provided classes.
8-12: DialogFooter.vue: Template layout is correct
The flexbox classes provide the intended responsive footer layout and the default slot allows custom button placement.unraid-ui/src/components/common/dialog/index.ts (1)
1-9: Central exports look good
Re-exporting all Dialog-related components from a single module simplifies imports elsewhere.unraid-ui/src/components/common/dialog/DialogTrigger.vue (1)
1-11: LGTM! Clean wrapper component implementation.This wrapper component correctly forwards props and slots to the underlying reka-ui DialogTrigger. The TypeScript typing is accurate and the implementation follows Vue 3 composition API best practices.
web/store/modal.ts (1)
11-11: Verify the initial modal visibility state.The refactor to use
useToggleis good, but initializing withtruemeans the modal starts visible. This seems unusual - typically modals start hidden.Please confirm this initial state is intentional. If modals should start hidden, consider:
- const [modalVisible, modalToggle] = useToggle(true); + const [modalVisible, modalToggle] = useToggle(false);unraid-ui/src/components/common/dialog/DialogTitle.vue (1)
1-21: LGTM! Excellent prop forwarding and class merging implementation.This component demonstrates best practices for extending reka-ui components:
- Proper use of
reactiveOmitto exclude theclassprop from forwarding- Correct application of
useForwardPropsfor reactive prop delegation- Clean class merging with the
cnutility- Appropriate TypeScript typing extending the base props
The implementation is solid and follows established patterns for wrapper components.
unraid-ui/src/components/common/dialog/DialogDescription.vue (1)
1-18: Well-structured dialog component wrapper.This implementation follows Vue 3 best practices with proper prop forwarding and CSS class merging. The use of
reactiveOmitanduseForwardPropsensures clean prop delegation to the underlyingreka-uicomponent while allowing custom class styling.api/generated-schema.graphql (2)
900-901: Schema extension for API key updates looks correct.The new
updatemutation follows the established pattern in theApiKeyMutationstype, acceptingUpdateApiKeyInputand returningApiKeyWithSecret.
935-941: Well-designed input type for partial updates.The
UpdateApiKeyInputtype correctly defines all fields as optional except for the requiredid, enabling flexible partial updates of API keys.api/src/unraid-api/graph/resolvers/validation.utils.ts (2)
4-10: Necessary imports for custom validation decorator.The expanded imports provide the required class-validator functionality for implementing custom decorators.
42-63: Robust custom validation decorator implementation.The
AtLeastOneOfdecorator is well-designed with:
- Clear validation logic checking for non-empty arrays
- Descriptive error messaging
- Proper use of class-validator's
registerDecoratorpatternThis will effectively enforce that API key operations include either roles or permissions.
api/src/unraid-api/auth/api-key.service.ts (2)
160-160: Good practice: immediate cache consistency.Adding the newly created API key to the memory cache immediately after saving ensures consistency between disk storage and in-memory state.
380-414: Well-implemented update method with proper validation.The update method demonstrates good practices:
- Validates API key existence before attempting updates
- Sanitizes name input using existing
sanitizeNamemethod- Validates roles against the allowed set
- Supports conditional updates for flexible partial modifications
- Saves changes to disk after validation
The implementation is consistent with the existing service patterns.
api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts (2)
16-16: LGTM!The import of
UpdateApiKeyInputis correctly added to support the new update functionality.
87-103: Well-implemented update method!The implementation follows established patterns with proper authorization, validation, and service layer interaction. The role synchronization after update is a good practice for maintaining consistency.
api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts (2)
16-16: LGTM!The import of
AtLeastOneOfvalidation decorator is correctly added to support the enhanced validation logic.
120-153: Well-structured update input class!The implementation correctly mirrors
CreateApiKeyInputwith appropriate changes for update operations:
- Required
idfield for identifying the target API key- Optional fields for partial updates
- Consistent validation decorators and patterns
- Same cross-field validation for roles/permissions
The structure follows GraphQL and NestJS best practices.
unraid-ui/src/components/common/dialog/DialogScrollContent.vue (3)
1-22: Excellent script setup structure!The implementation demonstrates best practices:
- Proper TypeScript typing with extension of
DialogContentProps- Clean separation of
classprop usingreactiveOmit- Correct forwarding of props and emits with
useForwardPropsEmits- Well-organized imports
The approach allows for flexible styling while maintaining prop forwarding.
24-61: Well-designed template structure!The implementation includes excellent features:
- Proper modal layering with
DialogPortalandDialogOverlay- Smooth animations using data-state attributes
- Responsive design with mobile-first approach
- Accessible close button with screen reader support
- Clean styling with Tailwind classes
The component provides a polished user experience.
37-48: Verify the pointer-down-outside behavior.The custom logic prevents dialog closing when clicking outside the content bounds (likely targeting scrollbar clicks). While technically correct, this deviates from standard modal behavior where outside clicks should close the dialog.
Please confirm this is the intended UX pattern, as users typically expect clicking outside a modal to close it.
If this behavior is intentional, consider adding a comment explaining the rationale:
@pointer-down-outside=" (event) => { + // Prevent closing when clicking on scrollbars or outside visible content area const originalEvent = event.detail.originalEvent;web/components/ApiKey/apikey.query.ts (4)
3-30: Excellent fragment implementation!The GraphQL fragments provide great benefits:
- Centralized field selection for consistency
- Clear naming convention (
ApiKeyvsApiKeyWithKey)- Proper nesting for permissions with resource and actions
- Reusability across queries and mutations
This approach reduces duplication and maintains consistency across operations.
35-35: Good refactoring to use fragments!Using the
...ApiKeyfragment in the query eliminates field duplication and ensures consistency with other operations.
44-48: Consistent fragment usage in mutations!Using
...ApiKeyWithKeyfragment ensures bothCREATE_API_KEYandUPDATE_API_KEYmutations return identical field sets, improving consistency and maintainability.
50-58: Well-implemented UPDATE_API_KEY mutation!The mutation follows the established pattern:
- Consistent structure with
CREATE_API_KEY- Proper use of
UpdateApiKeyInputtype- Returns
ApiKeyWithKeyfragment for consistency- Clean GraphQL syntax
This completes the CRUD operations for API key management.
web/components/ApiKey/ApiKeyModal.vue (1)
40-86: LGTM! Well-structured dialog implementation.The template structure is clean and follows good practices:
- Proper use of Dialog components from the UI library
- Conditional title rendering for create/edit modes
- Appropriate loading state management
- Clean event handling for modal interactions
web/composables/gql/gql.ts (2)
19-23: LGTM! Good refactoring to fragment-based GraphQL documents.The changes improve code organization and maintainability:
- Centralized API key field definitions in reusable fragments
- Consistent pattern for the new UpdateApiKey mutation
- Proper separation between ApiKey and ApiKeyWithKey fragments
114-130: LGTM! Function overloads correctly match the updated document structure.The function overloads are properly updated to support:
- New fragment definitions
- Fragment-based queries and mutations
- The new UpdateApiKey mutation
Type safety is maintained throughout the changes.
web/composables/gql/graphql.ts (3)
1500-1506: LGTM! Well-defined UpdateApiKeyInput type.The input type properly defines:
- Required
idfield for identifying the key to update- Optional fields that can be updated (
name,description,roles,permissions)- Consistent with the pattern of other input types
145-147: LGTM! Proper integration of update mutation into API key mutations.The update field is correctly added to the ApiKeyMutations type with:
- Appropriate return type (ApiKeyWithSecret)
- Proper input parameter typing
- Consistent with other mutation patterns
Also applies to: 174-177
1846-1848: LGTM! Fragment types and documents are properly generated.The fragment definitions correctly encapsulate the API key fields and maintain consistency between ApiKey and ApiKeyWithKey variants.
Also applies to: 2051-2052
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/helpers/functions.ts (1)
4-21: Improve readability with optional chaining.The function correctly handles error extraction from GraphQL errors with proper fallbacks. However, you can simplify the code using optional chaining as suggested by the static analysis tool.
Apply this diff to use optional chaining:
- const graphQLErrors = Array.isArray(e?.graphQLErrors) ? e.graphQLErrors : undefined; - if (graphQLErrors && graphQLErrors.length) { + if (Array.isArray(e?.graphQLErrors) && e.graphQLErrors.length) { + const graphQLErrors = e.graphQLErrors; const gqlError = graphQLErrors[0] as { extensions?: { originalError?: { message?: string[] } }; message?: string };🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/components/ApiKey/ApiKeyCreate.vue(5 hunks)web/components/ApiKey/ApiKeyManager.vue(3 hunks)web/components/ApiKey/ApiKeyModal.vue(1 hunks)web/helpers/functions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/components/ApiKey/ApiKeyModal.vue
- web/components/ApiKey/ApiKeyManager.vue
🧰 Additional context used
🪛 Biome (1.9.4)
web/helpers/functions.ts
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (11)
web/helpers/functions.ts (1)
2-2: LGTM!The function logic remains correct and unchanged.
web/components/ApiKey/ApiKeyCreate.vue (10)
2-2: LGTM!Import addition is appropriate for the new functionality.
20-27: LGTM!The new imports properly support the enhanced create/edit functionality with correct type imports and the error handling helper.
30-33: LGTM!Props are correctly typed and the optional
editingKeyprop enables the dual create/edit functionality.
38-46: LGTM!The reactive properties are properly typed and the computed properties correctly combine loading and error states from both mutations.
47-72: LGTM!The watch function correctly prefills form fields when editing and properly uses the fragment pattern for type safety. The immediate option ensures initialization works correctly.
75-91: LGTM!Permission toggling logic correctly handles type casting and maintains proper state management.
134-174: LGTM! Previous review feedback addressed.The
upsertKeyfunction now properly implements try/finally blocks to ensure the loading state is always cleared, addressing the previous review comment. The conditional logic for create vs update operations is correct, and the response handling properly extracts the result based on the operation type.
176-181: LGTM!Exposing the reactive properties allows parent components to access the internal state properly.
210-210: LGTM!The PermissionCounter component integration provides better UX by showing permission counts in the accordion trigger.
270-270: LGTM!Using the
extractGraphQLErrorMessagehelper provides better error messaging to users.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/components/ApiKey/ApiKeyManager.vue(3 hunks)web/components/ApiKey/ApiKeyModal.vue(1 hunks)web/components/Modals.ce.vue(2 hunks)web/store/modal.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/components/ApiKey/ApiKeyManager.vue
- web/store/modal.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
web/components/Modals.ce.vue (2)
7-7: LGTM! Proper import integration.The ApiKeyModal import correctly integrates the new modal component.
23-23: LGTM! Clean modal integration.The ApiKeyModal component is properly added to the modals container.
web/components/ApiKey/ApiKeyModal.vue (4)
1-29: LGTM! Well-structured component setup.The imports, store integration, and GraphQL query setup are properly implemented. The computed properties have appropriate fallbacks.
30-40: LGTM! Clean event handling and modal lifecycle management.The component ref, close handler, and created event handler are well-implemented with proper store integration.
42-64: LGTM! Proper dialog implementation with good UX.The dialog structure uses appropriate UI components with proper event binding and dynamic title based on edit/create mode.
65-77:Details
✅ Verification successful
Verify child component interface to prevent runtime errors.
The code assumes
apiKeyCreateRefexposesloading,postCreateLoadingproperties andupsertKey()method. Ensure these exist on the ApiKeyCreate component.
🏁 Script executed:
#!/bin/bash # Verify ApiKeyCreate component exposes the expected interface ast-grep --pattern $'export default defineComponent({ $$$ setup($$$) { $$$ return { $$$ loading: $_, $$$ postCreateLoading: $_, $$$ upsertKey: $_, $$$ } $$$ } $$$ })' # Also check for defineExpose usage rg -A 10 "defineExpose.*(?:loading|postCreateLoading|upsertKey)" web/components/ApiKey/ApiKeyCreate.vueLength of output: 339
🏁 Script executed:
#!/bin/bash # Extract the <script> block from ApiKeyCreate.vue to verify exposed properties/methods awk '/<script/,/<\/script>/' web/components/ApiKey/ApiKeyCreate.vueLength of output: 5880
ApiKeyCreate exposes the required interface
The
ApiKeyCreate.vuecomponent usesdefineExposeto exposeupsertKey,loading, andpostCreateLoading, so the usage inApiKeyModal.vueis safe and won’t cause runtime errors.
No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
web/components/ApiKey/actionVariant.ts (1)
18-29: Consider input validation and more specific typing.The function doesn't handle edge cases like empty strings or null inputs. Consider using a more specific type for the action parameter.
+type ApiKeyAction = 'read' | 'create' | 'update' | 'delete'; + -export function actionVariant(action: string): 'blue' | 'green' | 'yellow' | 'pink' | 'gray' { +export function actionVariant(action: string): 'blue' | 'green' | 'yellow' | 'pink' | 'gray' { + if (!action || typeof action !== 'string') { + return 'gray'; + } + switch (action) { case 'read': return 'blue';web/components/ApiKey/PermissionCounter.vue (2)
22-25: Remove unnecessary null check.The null check for
props.possiblePermissionsis unnecessary since the default value is already an empty array.const possibleActions = computed(() => { - if (!props.possiblePermissions) return []; return Array.from(new Set(props.possiblePermissions.flatMap((p) => p.actions))); });
27-37: Optimize the action counting logic.The current nested loop approach with
filter().lengthis inefficient. Consider a more direct counting approach.const actionCounts = computed(() => { const actions = possibleActions.value; const counts: Record<string, number> = {}; for (const action of actions) { - counts[action] = props.permissions.reduce( - (sum, perm) => sum + perm.actions.filter((a) => a === action).length, - 0 - ); + counts[action] = 0; + for (const perm of props.permissions) { + for (const permAction of perm.actions) { + if (permAction === action) { + counts[action]++; + } + } + } } return counts; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
unraid-ui/src/components/common/accordion/AccordionItem.vue(1 hunks)unraid-ui/src/components/common/accordion/AccordionTrigger.vue(1 hunks)unraid-ui/src/components/common/button/button.variants.ts(1 hunks)unraid-ui/src/components/common/dialog/DialogScrollContent.vue(1 hunks)web/components/ApiKey/ApiKeyCreate.vue(5 hunks)web/components/ApiKey/ApiKeyManager.vue(3 hunks)web/components/ApiKey/ApiKeyModal.vue(1 hunks)web/components/ApiKey/PermissionCounter.vue(1 hunks)web/components/ApiKey/actionVariant.ts(1 hunks)web/components/Modals.ce.vue(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/components/Modals.ce.vue
🚧 Files skipped from review as they are similar to previous changes (4)
- unraid-ui/src/components/common/dialog/DialogScrollContent.vue
- web/components/ApiKey/ApiKeyManager.vue
- web/components/ApiKey/ApiKeyModal.vue
- web/components/ApiKey/ApiKeyCreate.vue
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
unraid-ui/src/components/common/accordion/AccordionItem.vue (1)
15-15: LGTM! Improved styling flexibility.Removing the fixed
'border-b'class allows parent components to have full control over accordion item styling, which aligns with the broader UI updates for the API key management feature.unraid-ui/src/components/common/accordion/AccordionTrigger.vue (1)
19-19: LGTM! Enhanced accordion trigger styling.The styling updates create a more polished accordion trigger with bordered, rounded appearance and improved interaction states. The transition from hover underline to border color changes provides better visual feedback.
web/components/ApiKey/PermissionCounter.vue (1)
40-53: LGTM! Clean template implementation.The template logic is well-structured with proper conditional rendering and clear data binding. The use of the
actionVariantfunction for badge styling provides consistent visual representation.unraid-ui/src/components/common/button/button.variants.ts (1)
16-16:Details
❓ Verification inconclusive
Verify button height consistency across the application.
The change from fixed height (
h-9) to padding-based height (py-1) for small buttons will make their height content-dependent rather than consistent. This could affect visual alignment in layouts that rely on uniform button heights.Please verify that this change doesn't break visual consistency in existing UI layouts, especially in button groups or forms where uniform heights are expected.
🏁 Script executed:
#!/bin/bash # Description: Search for small button usage patterns to assess potential layout impact # Expected: Find components using sm size buttons to verify layout consistency # Search for small button size usage rg -A 3 -B 3 'size.*["\']sm["\']' --type ts --type tsx --type vue # Search for button groups or layouts that might depend on consistent heights rg -A 5 -B 5 'flex.*gap|grid.*gap' --type ts --type tsx --type vue | rg -A 5 -B 5 -i 'button'Length of output: 116
Let’s correct the quoting and broaden the search slightly:
🏁 Script executed:
#!/bin/bash # Re-run with fixed patterns to catch size="sm" usages and flex+gap layouts # 1) Find all Button components with size="sm" or size='sm' rg -A3 -B3 'size *= *"sm"' --type ts --type tsx --type vue rg -A3 -B3 "size *= *'sm'" --type ts --type tsx --type vue # 2) Locate any flex+gap containers and see if they hold Button components rg -A5 -B5 '<(div|section|form)[^>]*class(Name)?="[^"]*flex[^"]*gap-[^"]*"' --type tsx \ | rg -A5 -B5 -i '<Button'Length of output: 326
Let’s remove the
--typeflags and use globs instead:
🏁 Script executed:
#!/bin/bash # 1) Find all Button components with size="sm" or size='sm' rg -A3 -B3 "size\s*=\s*['\"]sm['\"]" -g "*.ts*" -g "*.vue" # 2) Locate flex+gap containers and see if they include <Button> rg -A5 -B5 "<(div|section|form)[^>]*class(Name)?=\"[^\"]*(flex|gap-)[^\"]*\"" -g "*.ts*" -g "*.vue" \ | rg -A5 -B5 -i "<Button"Length of output: 15733
Double-check small button height in grouped layouts
The small (
sm) variant was changed from a fixed height (h-9) to padding-based sizing (py-1), making its height content-driven. Please verify that this doesn’t introduce misalignment in flex/gap containers, particularly:
unraid-ui/src/components/ApiKey/ApiKeyManager.vue
•<div class="flex gap-2 flex-shrink-0">wrapping twosize="sm"buttonsweb/components/Notifications/Sidebar.vue
• Note this one usesclass="p-0 h-auto", so it’s explicitly overridden- Any other
size="sm"buttons in flex or grid layouts (e.g., forms, toolbars)Run a quick visual check in Storybook or relevant pages to ensure uniform alignment.
elibosley
force-pushed
the
feat/api-key-management-page
branch
from
cdcebd2 to
772d1cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/helpers/functions.ts (1)
7-21: Well-implemented error extraction with minor improvements possible.The function provides excellent error handling with proper fallbacks. However, there are two small improvements to consider:
- Apply static analysis suggestion: Use optional chaining for cleaner code
- Improve consistency: The error type checking mixes
errandereferencesApply this diff to address both issues:
- const graphQLErrors = Array.isArray(e?.graphQLErrors) ? e.graphQLErrors : undefined; - if (graphQLErrors && graphQLErrors.length) { + const graphQLErrors = Array.isArray(e?.graphQLErrors) ? e.graphQLErrors : undefined; + if (graphQLErrors?.length) { const gqlError = graphQLErrors[0] as { extensions?: { originalError?: { message?: string[] } }; message?: string }; message = gqlError?.extensions?.originalError?.message?.[0] || gqlError?.message || message; - } else if (typeof err === 'object' && err !== null && 'message' in err && typeof e.message === 'string') { + } else if (typeof e === 'object' && e !== null && 'message' in e && typeof e.message === 'string') { message = e.message; }🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
api/generated-schema.graphql(2 hunks)api/src/unraid-api/auth/api-key.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts(4 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts(3 hunks)api/src/unraid-api/graph/resolvers/validation.utils.ts(2 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/ApiKeys.page(1 hunks)unraid-ui/src/components/common/accordion/AccordionItem.vue(1 hunks)unraid-ui/src/components/common/accordion/AccordionTrigger.vue(1 hunks)unraid-ui/src/components/common/button/button.variants.ts(1 hunks)unraid-ui/src/components/common/dialog/Dialog.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogClose.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogContent.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogDescription.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogFooter.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogHeader.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogScrollContent.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogTitle.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogTrigger.vue(1 hunks)unraid-ui/src/components/common/dialog/index.ts(1 hunks)web/__test__/store/modal.test.ts(1 hunks)web/components/ApiKey/ApiKeyCreate.vue(5 hunks)web/components/ApiKey/ApiKeyManager.vue(3 hunks)web/components/ApiKey/ApiKeyModal.vue(1 hunks)web/components/ApiKey/PermissionCounter.vue(1 hunks)web/components/ApiKey/actionVariant.ts(1 hunks)web/components/ApiKey/apikey.query.ts(2 hunks)web/components/ApiKeyPage.ce.vue(1 hunks)web/components/Modals.ce.vue(2 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(5 hunks)web/helpers/functions.ts(1 hunks)web/nuxt.config.ts(1 hunks)web/pages/apikey.vue(1 hunks)web/pages/index.vue(1 hunks)web/pages/webComponents.vue(1 hunks)web/store/modal.ts(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/ApiKeys.page
- api/generated-schema.graphql
- web/pages/webComponents.vue
- web/components/ApiKey/actionVariant.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- unraid-ui/src/components/common/accordion/AccordionItem.vue
- unraid-ui/src/components/common/accordion/AccordionTrigger.vue
- unraid-ui/src/components/common/dialog/DialogHeader.vue
- api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts
- unraid-ui/src/components/common/dialog/DialogFooter.vue
- unraid-ui/src/components/common/dialog/DialogDescription.vue
- unraid-ui/src/components/common/dialog/index.ts
- api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts
- api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts
- api/src/unraid-api/auth/api-key.service.ts
- unraid-ui/src/components/common/dialog/DialogClose.vue
- unraid-ui/src/components/common/dialog/DialogScrollContent.vue
- web/components/ApiKeyPage.ce.vue
- api/src/unraid-api/graph/resolvers/validation.utils.ts
- web/pages/index.vue
- web/components/ApiKey/apikey.query.ts
- web/components/ApiKey/ApiKeyModal.vue
- unraid-ui/src/components/common/dialog/DialogTitle.vue
- unraid-ui/src/components/common/dialog/DialogTrigger.vue
- web/components/ApiKey/PermissionCounter.vue
- web/components/Modals.ce.vue
- web/components/ApiKey/ApiKeyCreate.vue
- web/test/store/modal.test.ts
- web/components/ApiKey/ApiKeyManager.vue
- unraid-ui/src/components/common/button/button.variants.ts
- unraid-ui/src/components/common/dialog/Dialog.vue
- web/nuxt.config.ts
- web/store/modal.ts
- web/composables/gql/gql.ts
- web/pages/apikey.vue
- unraid-ui/src/components/common/dialog/DialogContent.vue
- web/composables/gql/graphql.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/helpers/functions.ts
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
web/helpers/functions.ts (1)
2-2: LGTM! Clean and functional implementation.The
OBJ_TO_STRfunction correctly converts object key-value pairs to a formatted string.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unraid-ui/src/components/common/dialog/DialogClose.vue (1)
1-8: Good implementation of prop/event forwarding, but consider proper emit typing.The component has been updated to address the previous feedback by implementing
useForwardPropsEmits, which is excellent! However, the emits are defined as an empty array rather than using the proper TypeScript types.If
DialogClosefrom reka-ui has any events, consider importingDialogCloseEmitsand typing the emits properly:-import { DialogClose, useForwardPropsEmits, type DialogCloseProps } from 'reka-ui'; +import { DialogClose, useForwardPropsEmits, type DialogCloseProps, type DialogCloseEmits } from 'reka-ui'; -const emits = defineEmits([]); +const emits = defineEmits<DialogCloseEmits>();This ensures full type safety and proper event forwarding if the underlying component emits any events.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unraid-ui/src/components/common/dialog/DialogClose.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
unraid-ui/src/components/common/dialog/DialogClose.vue (1)
10-14: Template implementation looks good.The template correctly uses the forwarded props and provides slot support for flexible content composition.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/store/apiKey.ts (1)
6-13: Minor: tighten theshowModalAPI
showModalcurrently acceptsApiKeyFragment | null, but callers pass either
ApiKeyFragmentorApiKeyWithKeyFragment(seeApiKeyManager.vue). Narrowing the
argument withas ApiKeyFragmentthrows away thekeyfield and defeats type checking.- function showModal(key: ApiKeyFragment | null = null) { + function showModal(key: ApiKeyFragment | ApiKeyWithKeyFragment | null = null) {No runtime impact, but it protects future refactors.
web/components/ApiKey/ApiKeyManager.vue (1)
40-47: Remove leftoverconsole.logstatements
console.logcalls are handy during development but create noisy browser logs in production.- console.log(createdKey.value); + // console.debug('createdKey', createdKey.value) // ← keep if absolutely necessarySame for the log in
ApiKeyCreate.vue(lines 183/187).
Consider switching to a proper logger if you need runtime diagnostics.Also applies to: 183-188
web/components/ApiKey/ApiKeyCreate.vue (1)
180-189: Trim debug output & favour the helper for closingTwo quick clean-ups:
- Remove
console.log('fragmentData', …)– it leaks secrets (key) to the dev-tools console.- Instead of directly mutating
modalVisible/editingKey, you already haveclose()which wrapsapiKeyStore.hideModal(). Re-use it to keep intent consistent.- console.log('fragmentData', fragmentData); - apiKeyStore.setCreatedKey(fragmentData); + apiKeyStore.setCreatedKey(fragmentData);- modalVisible.value = false; - editingKey.value = null; + close();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/dev/Unraid.net/myservers.cfg(1 hunks)api/dev/configs/connect.json(0 hunks)unraid-ui/src/components.ts(1 hunks)web/components/ApiKey/ApiKeyCreate.vue(3 hunks)web/components/ApiKey/ApiKeyManager.vue(2 hunks)web/components/Modals.ce.vue(2 hunks)web/store/apiKey.ts(1 hunks)web/store/modal.ts(1 hunks)
💤 Files with no reviewable changes (1)
- api/dev/configs/connect.json
✅ Files skipped from review due to trivial changes (2)
- api/dev/Unraid.net/myservers.cfg
- unraid-ui/src/components.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/components/Modals.ce.vue
🧰 Additional context used
🧠 Learnings (2)
web/store/apiKey.ts (1)
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:52:57.888Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files ensures that all web components share a single Pinia store instance, which is the desired behavior. Without this initialization, each web component would have its own isolated store, breaking the intended architecture.
web/store/modal.ts (4)
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:52:57.888Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files ensures that all web components share a single Pinia store instance, which is the desired behavior. Without this initialization, each web component would have its own isolated store, breaking the intended architecture.
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:52:57.888Z
Learning: In the unraid/api project, Vue components are compiled into web components. The `setActivePinia(createPinia())` call at the module level in store files is intentional and ensures all web components share a single Pinia store instance, which is the desired behavior. This shared state approach is critical for the application's architecture to function correctly.
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:33:13.215Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files would break the build by causing all web components to share a singular Pinia store instance. Each web component needs its own Pinia store instance to function correctly.
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:33:13.215Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files would break the build by causing all web components to share a singular Pinia store instance. Each web component needs its own Pinia store instance to maintain proper isolation and encapsulation.
🧬 Code Graph Analysis (1)
web/store/apiKey.ts (1)
web/composables/gql/graphql.ts (2)
ApiKeyFragment(1945-1945)ApiKeyWithKeyFragment(1947-1947)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
web/store/modal.ts (1)
10-12: Confirm global Pinia instance is intentionalCalling
setActivePinia(createPinia())at module-load time makes every consumer of this store share the same global Pinia instance.
Your team’s learnings on this topic are split, so please double-check that this is indeed the desired behaviour for your web-component build.
Nothing is wrong with the newuseToggletuple—just make sure the single-store assumption won’t back-fire when multiple custom-elements are used on the same page.
| </div> | ||
| <div class="mb-2"> | ||
| <Label for="api-key-permissions">Permissions</Label> | ||
| <Accordion id="api-key-permissions" type="single" collapsible class="w-full mt-2"> |
There was a problem hiding this comment.
I know this will be getting refactored at some point and it's just for plugin devs, but this Accordion should probably still be a separate component in web since it's already being re-used. It would make the refactor easier when the new design is ready.
elibosley
force-pushed
the
feat/api-key-management-page
branch
from
eb2ada5 to
ea59f87
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
web/components/ApiKey/ApiKeyManager.vue (4)
3-3: UnnecessarystoreToRefsimportThe build tooling auto-imports common Pinia helpers (see project learning note).
Removing the explicit import avoids duplicate typings and keeps the import section lean.-import { storeToRefs } from 'pinia';
41-41: Remove strayconsole.logLeft-over debug logging clutters the browser console.
- console.log(createdKey.value);
79-80: Prefer UI modal overwindow.confirm
window.confirmblocks the UI and feels inconsistent with the rest of the design system.
Consider re-using the existing modal component for a smoother UX.
187-193:copiedflag is global
useClipboardreturns a singlecopiedref. After copying one key, the tooltip for every key shows “Copied!”.
InstantiateuseClipboardper key or store the copied ID to give accurate feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/dev/states/myservers.cfg(1 hunks)api/src/unraid-api/auth/api-key.service.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts(5 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.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)web/components/ApiKey/ApiKeyManager.vue(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- 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/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
- api/dev/states/myservers.cfg
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts
- api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts
🧰 Additional context used
🧠 Learnings (1)
web/components/ApiKey/ApiKeyManager.vue (1)
Learnt from: pujitm
PR: unraid/api#1417
File: web/components/ConnectSettings/ConnectSettings.ce.vue:11-18
Timestamp: 2025-06-13T17:14:21.739Z
Learning: The project’s build tooling auto-imports common Vue/Pinia helpers such as `storeToRefs`, so explicit import statements for them are not required.
🔇 Additional comments (2)
api/src/unraid-api/auth/api-key.service.spec.ts (1)
537-543: Verify thatAuthActionVerb.UPDATEexists
AuthActionVerb(imported fromnest-authz) historically exposesREAD | WRITE | DELETE | EXEC, notUPDATE.
IfUPDATEis absent the test silently compiles (because of the string enum fallback) but production code will reject the value.Confirm the enum or switch to an existing verb:
- actions: [AuthActionVerb.READ, AuthActionVerb.UPDATE], + actions: [AuthActionVerb.READ, AuthActionVerb.WRITE],web/components/ApiKey/ApiKeyManager.vue (1)
118-120:id.split(':')[1]may throwNot all IDs are guaranteed to contain a colon.
Guard or use a safer helper to avoidundefinedor runtime errors.- <span class="text-sm truncate"><b>ID:</b> {{ key.id.split(':')[1] }}</span> + <span class="text-sm truncate"><b>ID:</b> {{ key.id.includes(':') ? key.id.split(':')[1] : key.id }}</span>
| vi.spyOn(apiKeyService, 'loadAllFromDisk').mockResolvedValue([updateMockApiKey]); | ||
| vi.spyOn(apiKeyService, 'saveApiKey').mockResolvedValue(); | ||
| apiKeyService.onModuleInit(); | ||
| }); |
There was a problem hiding this comment.
Await onModuleInit() to avoid race conditions
ApiKeyService.onModuleInit() is async.
Calling it without await means the service may still be initialising when the first update assertion runs, producing flakiness on slower CI runners.
- apiKeyService.onModuleInit();
+ await apiKeyService.onModuleInit();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.spyOn(apiKeyService, 'loadAllFromDisk').mockResolvedValue([updateMockApiKey]); | |
| vi.spyOn(apiKeyService, 'saveApiKey').mockResolvedValue(); | |
| apiKeyService.onModuleInit(); | |
| }); | |
| vi.spyOn(apiKeyService, 'loadAllFromDisk').mockResolvedValue([updateMockApiKey]); | |
| vi.spyOn(apiKeyService, 'saveApiKey').mockResolvedValue(); | |
| await apiKeyService.onModuleInit(); | |
| }); |
🤖 Prompt for AI Agents
In api/src/unraid-api/auth/api-key.service.spec.ts around lines 499 to 502, the
call to the async method onModuleInit() is not awaited, which can cause race
conditions and flaky tests. Fix this by adding an await before
apiKeyService.onModuleInit() to ensure the initialization completes before
proceeding with assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
unraid-ui/eslint.config.ts (1)
13-20: Rule added but corresponding plugin is missing – ESLint will error
@typescript-eslint/consistent-type-importsis now incommonRules, yet thepluginssection of both the .ts and .vue overrides omits the@typescript-eslintplugin.
Because each entry in a flat config is isolated, ESLint will throw:Definition for rule '@typescript-eslint/consistent-type-imports' was not foundPatch both override blocks:
plugins: { 'no-relative-import-paths': noRelativeImportPaths, prettier: prettier, import: importPlugin, + '@typescript-eslint': tseslint.plugin, // vue: vuePlugin, // keep in the .vue block },
🧹 Nitpick comments (1)
unraid-ui/eslint.config.ts (1)
152-156: Nit: decide whether the config should lint itself
Ignoringeslint.config.tsprevents endless bikeshedding but also hides config-file issues. Just making sure that’s intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
unraid-ui/eslint.config.ts(3 hunks)unraid-ui/src/components/common/badge/badge.variants.ts(1 hunks)unraid-ui/src/components/common/button/button.variants.ts(2 hunks)unraid-ui/src/components/common/dialog/Dialog.vue(1 hunks)unraid-ui/src/components/common/sheet/sheet.variants.ts(1 hunks)unraid-ui/src/types/badge.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- unraid-ui/src/types/badge.ts
- unraid-ui/src/components/common/sheet/sheet.variants.ts
- unraid-ui/src/components/common/badge/badge.variants.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- unraid-ui/src/components/common/button/button.variants.ts
- unraid-ui/src/components/common/dialog/Dialog.vue
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
unraid-ui/eslint.config.ts (2)
2-3:@ts-expect-erroris the right directive here – good upgrade
Switching from@ts-ignoreto@ts-expect-errordocuments intent and will alert you once the missing types land. Nice touch.
8-10: CJS ↔ ESM interop: verify the imported parser object
vue-eslint-parseris still shipped as CommonJS. Importing it via a default ESM import relies on Node’s interop rules; in some setups you’ll receive the module itself, in others the module will be under.default. If ESLint receives the wrong shape it blows up with “the parser could not be loaded”.Consider a defensive hand-off:
- parser: vueEslintParser, + parser: (vueEslintParser as any)?.default ?? vueEslintParser,Please run
npx eslint "**/*.vue"locally to confirm the parser loads.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
web/composables/gql/graphql.ts (1)
937-976: Plugin management mutations deserve richer responses
addPlugin,removePlugin(and their arg objects) currently return a bareBooleanwhose meaning flips between “needs manual restart” and “operation failed”.
That ambiguity forces callers to guess.Consider returning an explicit object:
type PluginOperationResult { success: Boolean! restartRequired: Boolean! errors: [String!] }This conveys intent, allows future extensibility (e.g. per-plugin status), and removes guesswork.
Also applies to: 1019-1021, 1215-1234, 1282-1284
🧹 Nitpick comments (19)
web/composables/gql/graphql.ts (2)
133-139: Consider renaming or documenting thepluginsfield for clarity
ApiConfig.pluginsnow returnsstring[], whileQuery.pluginsreturnsPlugin[].
Because both live in the same namespace, it is easy to confuse names with full metadata objects. Either:
- Rename the scalar list to something like
pluginNames, or- Add a short comment clarifying that it is only the package names.
Small naming tweaks here prevent accidental misuse later.
2016-2027: Duplicate fragment names can clash during build-time document merging
ApiKeyFragmentDocdefinesfragment ApiKey, andApiKeysDocumentembeds another identicalfragment ApiKey.
Some GraphQL tooling (graphql-codegen,apollo,urql) throwsFragment "ApiKey" already exists in this documentwhen documents are concatenated.If both fragments are needed, rename one (e.g.,
fragment ApiKeyFields) or import the shared fragment rather than redefining it inside the query document.Also applies to: 2231-2233
api/generated-schema.graphql (17)
229-234: Add field descriptions for AccessUrl
TheAccessUrltype’s fields lack descriptive comments, unlike other types in the schema. Adding doc comments will improve API discoverability and consistency.
236-243: Document URL_TYPE enum values
Each enum value inURL_TYPEshould have a description to clarify its intended use. This aligns with the existing style for enums elsewhere in the schema.
248-248: Specify URL scalar specification
Consider annotating theURLscalar with@specifiedByto point to RFC3986, matching how other custom scalars (e.g.,JSON) reference their specs.
1493-1498: Consolidate AccessUrlObject and AccessUrl
There are two similar types—AccessUrlandAccessUrlObject—with overlapping fields. Consider unifying them (or clearly documenting their distinct roles) to reduce confusion.
1500-1509: Add field descriptions for RemoteAccess
Fields inRemoteAccesslack documentation. Adding comments foraccessType,forwardType, andportwill match the overall schema’s level of detail.
1511-1515: Add descriptions for WAN_ACCESS_TYPE values
Document each value (DYNAMIC,ALWAYS,DISABLED) inWAN_ACCESS_TYPEso clients understand their behavior.
1517-1520: Add descriptions for WAN_FORWARD_TYPE values
EnumWAN_FORWARD_TYPEvalues (UPNP,STATIC) need field-level docs to clarify their semantics.
1522-1531: Add descriptions for DynamicRemoteAccessStatus fields
TheDynamicRemoteAccessStatustype should include comments forenabledType,runningType, anderrorto explain when and how they’re populated.
1533-1537: Add descriptions for DynamicRemoteAccessType values
EnumDynamicRemoteAccessTypeentries should be documented to clarify the differences betweenSTATIC,UPNP, andDISABLED.
1539-1548: Add descriptions for ConnectSettingsValues fields
Fields inConnectSettingsValuesmirrorRemoteAccessfields but lack docs. Adding descriptions ensures consistency and clarity.
1550-1561: Add descriptions for ConnectSettings type
TheConnectSettingstype’s fields (dataSchema,uiSchema,values) are undocumented here—adding comments will aid API consumers.
1563-1571: Add descriptions for Connect type
Fields inConnectneed brief doc comments to explain their purpose and usage.
1573-1576: Add descriptions for Network type
TheNetworktype and itsaccessUrlsfield should be documented for developer clarity.
1578-1581: Add descriptions for ApiKeyResponse fields
DocumentvalidanderrorinApiKeyResponseto define what each represents and when errors are returned.
1672-1676: Document new root Query fields
The addedremoteAccess,connect,network, andcloudquery fields lack descriptions. Please add comments to explain their returns.
1718-1723: Document new root Mutation fields
MutationsupdateApiSettings,connectSignIn,connectSignOut,setupRemoteAccess, andenableDynamicRemoteAccessneed schema-level docs for their parameters and expected behavior.
1794-1800: Use camelCase for input field names
The GraphQL convention is camelCase; consider renamingpreferred_usernametopreferredUsernamefor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
api/dev/configs/api.json(1 hunks)api/dev/configs/connect.json(0 hunks)api/generated-schema-new.graphql(0 hunks)api/generated-schema.graphql(7 hunks)api/src/unraid-api/graph/graph.module.ts(1 hunks)api/src/unraid-api/graph/validate-schema.ts(0 hunks)generated-schema-new.graphql(0 hunks)packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts(1 hunks)web/components/ConnectSettings/AllowedOrigins.vue(0 hunks)web/components/ConnectSettings/RemoteAccess.vue(0 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(15 hunks)web/store/unraidApiSettings.fragment.ts(0 hunks)
💤 Files with no reviewable changes (7)
- api/dev/configs/connect.json
- api/src/unraid-api/graph/validate-schema.ts
- web/store/unraidApiSettings.fragment.ts
- web/components/ConnectSettings/AllowedOrigins.vue
- generated-schema-new.graphql
- api/generated-schema-new.graphql
- web/components/ConnectSettings/RemoteAccess.vue
✅ Files skipped from review due to trivial changes (3)
- api/src/unraid-api/graph/graph.module.ts
- packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts
- api/dev/configs/api.json
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
web/composables/gql/gql.ts (3)
19-23: Fragments registered correctly
ApiKeyandApiKeyWithKeyfragments are added to theDocumentsmap with the propertypeofreferences. Type-safety for downstream consumers is preserved.
55-60:documentsconstant kept in syncThe runtime
documentsmap mirrors the new fragments and mutations, so look-ups at runtime will resolve as expected.
114-130: Overload signatures updatedNew overloads for the fragments,
ApiKeysquery, and bothCreateApiKey/UpdateApiKeymutations are present. This guards callers at compile-time against string typos.web/composables/gql/graphql.ts (1)
162-164: Let’s do a literal search for any remaining REST calls, then confirm the new GraphQL mutation is in use:#!/bin/bash # 1) Find any old REST calls rg -n -F 'updateApiKey(' # 2) Verify usage of the new GraphQL field rg -n 'apiKey\.update'api/generated-schema.graphql (7)
780-783: Verify returning secret in update mutation
Theupdatemutation returnsApiKeyWithSecret, exposing the key on every update. Confirm this is intentional and that it doesn’t inadvertently leak secrets when only metadata changes.
816-822: Ensure server-side validation enforces input
UpdateApiKeyInputallows all fields to be optional (aside fromid). Confirm that the resolver applies theAtLeastOneOfrule so updates without any actual changes are rejected.
1764-1775: ConnectSettingsInput SDL additions look good
Field docs are present, types align with backend expectations.
1777-1783: ConnectSignInInput SDL additions look good
Well-documented and structured according to the authentication flow.
1805-1816: SetupRemoteAccessInput SDL additions look good
Inputs and docs accurately reflect the remote‐access configuration requirements.
1818-1824: EnableDynamicRemoteAccessInput SDL additions look good
This input type is clear and aligns with the mutation’s needs.
1826-1831: AccessUrlInput SDL additions look good
Well-documented and matches theAccessUrloutput shape.
| export type ConnectSettingsInput = { | ||
| /** The type of WAN access to use for Remote Access */ | ||
| accessType?: InputMaybe<WanAccessType>; | ||
| /** The type of port forwarding to use for Remote Access */ | ||
| forwardType?: InputMaybe<WanForwardType>; | ||
| /** The port to use for Remote Access. Not required for UPNP forwardType. Required for STATIC forwardType. Ignored if accessType is DISABLED or forwardType is UPNP. */ | ||
| port?: InputMaybe<Scalars['Int']['input']>; | ||
| }; |
There was a problem hiding this comment.
Stale fields linger in ConnectSettingsInput
ConnectSettingsInput still exposes accessType, forwardType, and port, which were removed from the UI along with Remote-Access screens.
Keeping these fields:
- Increases surface for confused consumers
- Risks accidental re-exposure of deprecated behaviour on the server side
Recommend pruning them from the schema now rather than later.
🤖 Prompt for AI Agents
In web/composables/gql/graphql.ts around lines 470 to 477, the
ConnectSettingsInput type still includes accessType, forwardType, and port
fields that are no longer used in the UI and should be removed. To fix this,
delete these three fields from the ConnectSettingsInput type definition to
prevent confusion and avoid exposing deprecated functionality on the server
side.
elibosley
force-pushed
the
feat/api-key-management-page
branch
from
ec4948e to
609050b
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
web/composables/gql/graphql.ts (1)
470-477: Stale Remote-Access fields still present inConnectSettingsInput
Same issue flagged previously – fieldsaccessType,forwardType, andportare deprecated in UI but remain in the schema.
Please remove to avoid re-exposing dropped functionality.
🧹 Nitpick comments (12)
web/helpers/functions.ts (1)
10-13: Prefer optional-chaining to avoid the extra castYou can drop the intermediary cast and make the intent clearer by chaining directly on
err:-const e = err as { graphQLErrors?: unknown; message?: string }; -const graphQLErrors = Array.isArray(e?.graphQLErrors) ? e.graphQLErrors : undefined; +const graphQLErrors = Array.isArray((err as any)?.graphQLErrors) + ? (err as any).graphQLErrors + : undefined;This removes an unnecessary variable, satisfies the Biome hint, and keeps the function fully type-safe.
web/components/ApiKey/ApiKeyManager.vue (1)
41-42: Strip leftoverconsole.logbefore shipping
console.log(createdKey.value);is a debugging artefact that will leak into production builds and browser consoles. Please remove it (and consider adding an ESLint rule to block accidental logs).web/components/ApiKey/ApiKeyCreate.vue (1)
181-189: Remove debug logs for cleaner production codeThe
console.log('fragmentData', …)statements are useful during development but noisy in production and may expose sensitive key material. Please delete them.- const fragmentData = useFragment(API_KEY_FRAGMENT_WITH_KEY, apiKeyResult.update); - console.log('fragmentData', fragmentData); - apiKeyStore.setCreatedKey(fragmentData); + const fragmentData = useFragment(API_KEY_FRAGMENT_WITH_KEY, apiKeyResult.update); + apiKeyStore.setCreatedKey(fragmentData); … - const fragmentData = useFragment(API_KEY_FRAGMENT_WITH_KEY, apiKeyResult.create); - console.log('fragmentData', fragmentData); - apiKeyStore.setCreatedKey(fragmentData); + const fragmentData = useFragment(API_KEY_FRAGMENT_WITH_KEY, apiKeyResult.create); + apiKeyStore.setCreatedKey(fragmentData);api/generated-schema.graphql (9)
236-243: NIT: ClarifyDEFAULTinURL_TYPEenum
TheDEFAULTvalue is ambiguous—consider renaming it toUNKNOWNor adding a description to explain its purpose.
248-248: Refactor: Add@specifiedBydirective toURLscalar
To document RFC3986 compliance, annotatescalar URLwith@specifiedBy(url: "https://www.ietf.org/rfc/rfc3986.txt").
816-822: Enhancement: Enforce non-empty updates inUpdateApiKeyInput
Consider adding a GraphQL validation directive (e.g.@atLeastOneOf(fields: ["name","description","roles","permissions"])) to ensure the input carries at least one change.
1500-1509: NIT: DocumentforwardTypeandportinRemoteAccess
Add field-level descriptions explaining whenportis required (e.g., “Required for STATIC forwarding”). Improves self-documentation.
1517-1520: NIT: Add doc comments toWAN_FORWARD_TYPEenum
Describe each strategy (e.g., UPNP vs STATIC) for clarity in the schema.
1539-1548: Refactor: DeduplicateConnectSettingsValuesandRemoteAccess
Both types share fields (accessType,forwardType,port). Consider extracting a common interface or reusing one type to reduce duplication.
1573-1576: Refactor:Network.accessUrlsnullability
Schema uses[AccessUrl!]but allowsaccessUrlsto be null. Consider[AccessUrl!]!for consistency with other non-null list fields.
1777-1793: Enhancement: ValidateConnectSignInInputcombinations
Consider enforcing exactly one ofidTokenor (accessToken+refreshToken), or clarifying expected auth flows in docs.
1805-1816: Refactor: Consolidate remote-access inputs
SetupRemoteAccessInputduplicatesConnectSettingsInput. Consider reusing or extending a single input type to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
api/dev/configs/api.json(1 hunks)api/dev/configs/connect.json(0 hunks)api/generated-schema-new.graphql(0 hunks)api/generated-schema.graphql(7 hunks)api/src/unraid-api/auth/api-key.service.spec.ts(2 hunks)api/src/unraid-api/auth/api-key.service.ts(1 hunks)api/src/unraid-api/graph/graph.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts(4 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts(5 hunks)api/src/unraid-api/graph/resolvers/validation.utils.ts(2 hunks)api/src/unraid-api/graph/validate-schema.ts(0 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.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)generated-schema-new.graphql(0 hunks)packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/ApiKeys.page(1 hunks)unraid-ui/eslint.config.ts(3 hunks)unraid-ui/src/components.ts(1 hunks)unraid-ui/src/components/common/accordion/AccordionItem.vue(1 hunks)unraid-ui/src/components/common/accordion/AccordionTrigger.vue(1 hunks)unraid-ui/src/components/common/badge/badge.variants.ts(1 hunks)unraid-ui/src/components/common/button/button.variants.ts(2 hunks)unraid-ui/src/components/common/dialog/Dialog.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogClose.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogContent.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogDescription.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogFooter.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogHeader.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogScrollContent.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogTitle.vue(1 hunks)unraid-ui/src/components/common/dialog/DialogTrigger.vue(1 hunks)unraid-ui/src/components/common/dialog/index.ts(1 hunks)unraid-ui/src/components/common/sheet/sheet.variants.ts(1 hunks)unraid-ui/src/types/badge.ts(1 hunks)web/__test__/store/modal.test.ts(1 hunks)web/__test__/store/unraidApiSettings.test.ts(0 hunks)web/components/ApiKey/ApiKeyCreate.vue(3 hunks)web/components/ApiKey/ApiKeyManager.vue(2 hunks)web/components/ApiKey/PermissionCounter.vue(1 hunks)web/components/ApiKey/actionVariant.ts(1 hunks)web/components/ApiKey/apikey.query.ts(2 hunks)web/components/ApiKeyPage.ce.vue(1 hunks)web/components/ConnectSettings/AllowedOrigins.vue(0 hunks)web/components/ConnectSettings/RemoteAccess.vue(0 hunks)web/components/Modals.ce.vue(2 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(15 hunks)web/helpers/functions.ts(1 hunks)web/nuxt.config.ts(1 hunks)web/pages/apikey.vue(1 hunks)web/pages/index.vue(1 hunks)web/pages/webComponents.vue(1 hunks)web/store/apiKey.ts(1 hunks)web/store/modal.ts(1 hunks)web/store/unraidApiSettings.fragment.ts(0 hunks)web/store/unraidApiSettings.ts(0 hunks)
💤 Files with no reviewable changes (9)
- api/dev/configs/connect.json
- api/src/unraid-api/graph/validate-schema.ts
- web/components/ConnectSettings/AllowedOrigins.vue
- web/test/store/unraidApiSettings.test.ts
- web/store/unraidApiSettings.fragment.ts
- web/components/ConnectSettings/RemoteAccess.vue
- api/generated-schema-new.graphql
- web/store/unraidApiSettings.ts
- generated-schema-new.graphql
✅ Files skipped from review due to trivial changes (2)
- unraid-ui/src/components/common/dialog/DialogTrigger.vue
- web/components/ApiKey/actionVariant.ts
🚧 Files skipped from review as they are similar to previous changes (43)
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php.last-download-time
- api/src/unraid-api/graph/graph.module.ts
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/Notifications.page.last-download-time
- packages/unraid-api-plugin-connect/src/resolver/connect.resolver.ts
- unraid-ui/src/components/common/accordion/AccordionTrigger.vue
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
- web/test/store/modal.test.ts
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
- api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.spec.ts
- web/pages/apikey.vue
- api/dev/configs/api.json
- web/pages/webComponents.vue
- unraid-ui/src/components.ts
- unraid-ui/src/components/common/sheet/sheet.variants.ts
- unraid-ui/src/components/common/dialog/DialogDescription.vue
- web/pages/index.vue
- unraid-ui/src/components/common/badge/badge.variants.ts
- unraid-ui/src/components/common/dialog/DialogFooter.vue
- unraid-ui/src/types/badge.ts
- unraid-ui/src/components/common/button/button.variants.ts
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/ApiKeys.page
- unraid-ui/src/components/common/accordion/AccordionItem.vue
- unraid-ui/src/components/common/dialog/index.ts
- api/src/unraid-api/graph/resolvers/validation.utils.ts
- unraid-ui/src/components/common/dialog/Dialog.vue
- unraid-ui/eslint.config.ts
- web/components/Modals.ce.vue
- api/src/unraid-api/auth/api-key.service.ts
- web/components/ApiKeyPage.ce.vue
- unraid-ui/src/components/common/dialog/DialogContent.vue
- unraid-ui/src/components/common/dialog/DialogHeader.vue
- api/src/unraid-api/graph/resolvers/api-key/api-key.model.ts
- web/store/apiKey.ts
- unraid-ui/src/components/common/dialog/DialogTitle.vue
- api/src/unraid-api/graph/resolvers/api-key/api-key.mutation.ts
- web/store/modal.ts
- unraid-ui/src/components/common/dialog/DialogClose.vue
- web/components/ApiKey/PermissionCounter.vue
- web/nuxt.config.ts
- web/components/ApiKey/apikey.query.ts
- api/src/unraid-api/auth/api-key.service.spec.ts
- unraid-ui/src/components/common/dialog/DialogScrollContent.vue
- web/composables/gql/gql.ts
🧰 Additional context used
🧠 Learnings (1)
web/components/ApiKey/ApiKeyManager.vue (1)
Learnt from: pujitm
PR: unraid/api#1417
File: web/components/ConnectSettings/ConnectSettings.ce.vue:11-18
Timestamp: 2025-06-13T17:14:21.739Z
Learning: The project’s build tooling auto-imports common Vue/Pinia helpers such as `storeToRefs`, so explicit import statements for them are not required.
🪛 Biome (1.9.4)
web/helpers/functions.ts
[error] 11-11: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
web/composables/gql/graphql.ts (1)
135-136:pluginsfield addition looks goodThe new
plugins: String[]property neatly extendsApiConfigwithout breaking existing shape.
No further concerns.api/generated-schema.graphql (11)
229-234: Schema:AccessUrldefinition is correct and aligned with RFC3986
Fieldsipv4andipv6leverage the newURLscalar, ensuring URL validation. Nice work.
1511-1515: LGTM:WAN_ACCESS_TYPEenum
Values DYNAMIC, ALWAYS, DISABLED are well-defined and ordered logically.
1522-1531: LGTM:DynamicRemoteAccessStatusoutput type
Provides bothenabledTypeandrunningTypealong witherror. Good shape.
1533-1537: LGTM:DynamicRemoteAccessTypeenum
Consistent with other access-type enums and clearly named.
1550-1561: LGTM:ConnectSettingsfollows Node pattern
Non-nulldataSchema,uiSchema, andvaluesmatch other settings types.
1563-1571: LGTM:Connecttype is well-structured
Grouping dynamic status and settings under one node is clean.
1578-1581: LGTM:ApiKeyResponsetype
Simple and effective: flag success and optional error message.
1764-1775: LGTM:ConnectSettingsInputdocumentation
Detailed descriptions onportrequirements are very helpful.
1818-1825: LGTM:EnableDynamicRemoteAccessInputshape
Clear and concise:urlplusenabledflag.
1826-1831: LGTM:AccessUrlInputmatches output type
Maintains symmetry withAccessUrl—good for client code generation.
1718-1723: Refactor & Secure:updateApiSettingsnaming and permissions
- Rename to
updateConnectSettingsfor clarity.- Annotate all new mutations (
updateApiSettings,connectSignIn,connectSignOut,setupRemoteAccess,enableDynamicRemoteAccess) with appropriate@usePermissions.⛔ Skipped due to learnings
Learnt from: pujitm PR: unraid/api#1211 File: api/src/graphql/schema/types/connect/connect.graphql:142-146 Timestamp: 2025-03-14T16:10:38.562Z Learning: The updateApiSettings mutation in api/src/unraid-api/graph/connect/connect.resolver.ts is protected with the @UsePermissions decorator that requires UPDATE permission on the CONFIG resource.
| <ul v-if="apiKeys.length" class="flex flex-col gap-4 mb-6"> | ||
| <CardWrapper v-for="key in apiKeys" :key="key.id"> | ||
| <li class="flex flex-row items-start justify-between gap-4 p-4 list-none"> | ||
| <div class="flex-1 min-w-0"> | ||
| <header class="flex gap-2 justify-between items-start"> | ||
| <div class="flex flex-col gap-2"> | ||
| <span class="text-sm truncate"><b>ID:</b> {{ key.id.split(':')[1] }}</span> | ||
| <span class="text-sm truncate"><b>Name:</b> {{ key.name }}</span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invalid <ul> structure – list children aren’t <li> elements
v-for is placed on <CardWrapper> which likely renders a <div>, meaning the direct children of <ul> are <div>s, not <li>s. This breaks HTML semantics and screen-reader expectations.
Two quick fixes:
-<ul …>
- <CardWrapper v-for="key in apiKeys" :key="key.id">
- <li>…</li>
- </CardWrapper>
-</ul>
+<ul …>
+ <li v-for="key in apiKeys" :key="key.id" class="list-none">
+ <CardWrapper>
+ …content…
+ </CardWrapper>
+ </li>
+</ul>or expose a prop on CardWrapper so it renders <li> internally (<CardWrapper as="li">).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/components/ApiKey/ApiKeyManager.vue around lines 112 to 119, the <ul>
element has direct children rendered by <CardWrapper> which likely outputs <div>
elements, breaking HTML semantics since <ul> should only have <li> children. To
fix this, either move the v-for directive to the <li> element inside
<CardWrapper> so that <li> is the direct child of <ul>, or modify the
CardWrapper component to accept a prop that allows it to render as an <li>
element (e.g., <CardWrapper as="li">) ensuring valid list structure.
| /** Update an API key */ | ||
| update: ApiKeyWithSecret; | ||
| }; |
There was a problem hiding this comment.
Exposes API key secret on every update – potential security leak
Returning ApiKeyWithSecret from apiKey.update will expose the key value on every update call.
Industry-standard practice is to reveal the secret only once, at creation time. Subsequent operations should return ApiKey, omitting the key field.
- /** Update an API key */
- update: ApiKeyWithSecret;
+ /** Update an API key – secret is NOT returned */
+ update: ApiKey;Corresponding changes are required in:
ApiKeyMutationsUpdateArgsresponse mappingUpdateApiKeyMutationdocument & fragment usage- Backend resolver to withhold the secret
Failing to address this leaks credentials to any caller able to perform updates.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Update an API key */ | |
| update: ApiKeyWithSecret; | |
| }; | |
| /** Update an API key – secret is NOT returned */ | |
| update: ApiKey; | |
| }; |
🤖 Prompt for AI Agents
In web/composables/gql/graphql.ts around lines 162 to 164, the update method
currently returns ApiKeyWithSecret, exposing the API key secret on every update
call, which is a security risk. Change the return type of apiKey.update to
ApiKey (without the secret key field) to prevent exposing the secret after
creation. Also update ApiKeyMutationsUpdateArgs response mapping, the
UpdateApiKeyMutation document and fragment usage, and modify the backend
resolver to withhold the secret on update operations.
|
|
||
| """Update an API key""" | ||
| update(input: UpdateApiKeyInput!): ApiKeyWithSecret! |
There was a problem hiding this comment.
Critical: Missing permission directive on update mutation
The new update field under ApiKeyMutations lacks an authorization directive (@usePermissions). Add a directive like @usePermissions(action: UPDATE, resource: API_KEY, possession: OWN_ANY) to secure it.
🤖 Prompt for AI Agents
In api/generated-schema.graphql around lines 780 to 782, the `update` mutation
is missing the required authorization directive. Add the directive
`@usePermissions(action: UPDATE, resource: API_KEY, possession: OWN_ANY)` to the
`update` field to enforce proper permission checks and secure the mutation.
| type AccessUrlObject { | ||
| ipv4: String | ||
| ipv6: String | ||
| type: URL_TYPE! | ||
| name: String | ||
| } |
There was a problem hiding this comment.
Critical: Inconsistent URL field types in AccessUrlObject
AccessUrlObject.ipv4 and ipv6 are String, whereas AccessUrl uses URL. Switch to URL scalar here to enforce URL validation uniformly.
🤖 Prompt for AI Agents
In api/generated-schema.graphql around lines 1493 to 1498, the fields ipv4 and
ipv6 in AccessUrlObject are typed as String but should be changed to the URL
scalar type to match the AccessUrl type and enforce consistent URL validation.
Update the type declarations of ipv4 and ipv6 from String to URL.
| remoteAccess: RemoteAccess! | ||
| connect: Connect! | ||
| network: Network! | ||
| cloud: Cloud! | ||
| } |
There was a problem hiding this comment.
Critical: Secure new Query fields with permissions
Fields remoteAccess, connect, network, and cloud should each include @usePermissions(action: READ, resource: CONNECT__REMOTE_ACCESS, possession: OWN) (or appropriate resource verb).
🤖 Prompt for AI Agents
In api/generated-schema.graphql around lines 1672 to 1676, the fields
remoteAccess, connect, network, and cloud in the Query type lack permission
directives. Add the @usePermissions directive to each field with the appropriate
parameters, such as action: READ, resource: CONNECT__REMOTE_ACCESS, and
possession: OWN, to enforce access control and secure these query fields
properly.
| input ConnectUserInfoInput { | ||
| """The preferred username of the user""" | ||
| preferred_username: String! | ||
|
|
||
| """The email address of the user""" | ||
| email: String! | ||
|
|
||
| """The avatar URL of the user""" | ||
| avatar: String | ||
| } |
There was a problem hiding this comment.
Critical: Inconsistent naming in ConnectUserInfoInput
Fields use snake_case (preferred_username)—rename to camelCase (preferredUsername) to match GraphQL conventions.
🤖 Prompt for AI Agents
In api/generated-schema.graphql around lines 1794 to 1803, the input type
ConnectUserInfoInput uses snake_case for its fields like preferred_username,
which is inconsistent with GraphQL naming conventions. Rename all fields in this
input type to camelCase, for example, change preferred_username to
preferredUsername, to ensure consistency and follow standard GraphQL style.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.9.0](v4.8.0...v4.9.0) (2025-07-08) ### Features * add graphql resource for API plugins ([#1420](#1420)) ([642a220](642a220)) * add management page for API keys ([#1408](#1408)) ([0788756](0788756)) * add rclone ([#1362](#1362)) ([5517e75](5517e75)) * API key management ([#1407](#1407)) ([d37dc3b](d37dc3b)) * api plugin management via CLI ([#1416](#1416)) ([3dcbfbe](3dcbfbe)) * build out docker components ([#1427](#1427)) ([711cc9a](711cc9a)) * docker and info resolver issues ([#1423](#1423)) ([9901039](9901039)) * fix shading in UPC to be less severe ([#1438](#1438)) ([b7c2407](b7c2407)) * info resolver cleanup ([#1425](#1425)) ([1b279bb](1b279bb)) * initial codeql setup ([#1390](#1390)) ([2ade7eb](2ade7eb)) * initialize claude code in codebse ([#1418](#1418)) ([b6c4ee6](b6c4ee6)) * move api key fetching to use api key service ([#1439](#1439)) ([86bea56](86bea56)) * move to cron v4 ([#1428](#1428)) ([b8035c2](b8035c2)) * move to iframe for changelog ([#1388](#1388)) ([fcd6fbc](fcd6fbc)) * native slackware package ([#1381](#1381)) ([4f63b4c](4f63b4c)) * send active unraid theme to docs ([#1400](#1400)) ([f71943b](f71943b)) * slightly better watch mode ([#1398](#1398)) ([881f1e0](881f1e0)) * upgrade nuxt-custom-elements ([#1461](#1461)) ([345e83b](345e83b)) * use bigint instead of long ([#1403](#1403)) ([574d572](574d572)) ### Bug Fixes * activation indicator removed ([5edfd82](5edfd82)) * alignment of settings on ManagementAccess settings page ([#1421](#1421)) ([70c790f](70c790f)) * allow rclone to fail to initialize ([#1453](#1453)) ([7c6f02a](7c6f02a)) * always download 7.1 versioned files for patching ([edc0d15](edc0d15)) * api `pnpm type-check` ([#1442](#1442)) ([3122bdb](3122bdb)) * **api:** connect config `email` validation ([#1454](#1454)) ([b9a1b9b](b9a1b9b)) * backport unraid/webgui[#2269](https://github.com/unraid/api/issues/2269) rc.nginx update ([#1436](#1436)) ([a7ef06e](a7ef06e)) * bigint ([e54d27a](e54d27a)) * config migration from `myservers.cfg` ([#1440](#1440)) ([c4c9984](c4c9984)) * **connect:** fatal race-condition in websocket disposal ([#1462](#1462)) ([0ec0de9](0ec0de9)) * **connect:** mothership connection ([#1464](#1464)) ([7be8bc8](7be8bc8)) * console hidden ([9b85e00](9b85e00)) * debounce is too long ([#1426](#1426)) ([f12d231](f12d231)) * delete legacy connect keys and ensure description ([22fe91c](22fe91c)) * **deps:** pin dependencies ([#1465](#1465)) ([ba75a40](ba75a40)) * **deps:** pin dependencies ([#1470](#1470)) ([412b329](412b329)) * **deps:** storybook v9 ([#1476](#1476)) ([45bb49b](45bb49b)) * **deps:** update all non-major dependencies ([#1366](#1366)) ([291ee47](291ee47)) * **deps:** update all non-major dependencies ([#1379](#1379)) ([8f70326](8f70326)) * **deps:** update all non-major dependencies ([#1389](#1389)) ([cb43f95](cb43f95)) * **deps:** update all non-major dependencies ([#1399](#1399)) ([68df344](68df344)) * **deps:** update dependency @types/diff to v8 ([#1393](#1393)) ([00da27d](00da27d)) * **deps:** update dependency cache-manager to v7 ([#1413](#1413)) ([9492c2a](9492c2a)) * **deps:** update dependency commander to v14 ([#1394](#1394)) ([106ea09](106ea09)) * **deps:** update dependency diff to v8 ([#1386](#1386)) ([e580f64](e580f64)) * **deps:** update dependency dotenv to v17 ([#1474](#1474)) ([d613bfa](d613bfa)) * **deps:** update dependency lucide-vue-next to ^0.509.0 ([#1383](#1383)) ([469333a](469333a)) * **deps:** update dependency marked to v16 ([#1444](#1444)) ([453a5b2](453a5b2)) * **deps:** update dependency shadcn-vue to v2 ([#1302](#1302)) ([26ecf77](26ecf77)) * **deps:** update dependency vue-sonner to v2 ([#1401](#1401)) ([53ca414](53ca414)) * disable file changes on Unraid 7.2 ([#1382](#1382)) ([02de89d](02de89d)) * do not start API with doinst.sh ([7d88b33](7d88b33)) * do not uninstall fully on 7.2 ([#1484](#1484)) ([2263881](2263881)) * drop console with terser ([a87d455](a87d455)) * error logs from `cloud` query when connect is not installed ([#1450](#1450)) ([719f460](719f460)) * flash backup integration with Unraid Connect config ([#1448](#1448)) ([038c582](038c582)) * header padding regression ([#1477](#1477)) ([e791cc6](e791cc6)) * incorrect state merging in redux store ([#1437](#1437)) ([17b7428](17b7428)) * lanip copy button not present ([#1459](#1459)) ([a280786](a280786)) * move to bigint scalar ([b625227](b625227)) * node_modules dir removed on plugin update ([#1406](#1406)) ([7b005cb](7b005cb)) * omit Connect actions in UPC when plugin is not installed ([#1417](#1417)) ([8c8a527](8c8a527)) * parsing of `ssoEnabled` in state.php ([#1455](#1455)) ([f542c8e](f542c8e)) * pin ranges ([#1460](#1460)) ([f88400e](f88400e)) * pr plugin promotion workflow ([#1456](#1456)) ([13bd9bb](13bd9bb)) * proper fallback if missing paths config modules ([7067e9e](7067e9e)) * rc.unraid-api now cleans up older dependencies ([#1404](#1404)) ([83076bb](83076bb)) * remote access lifecycle during boot & shutdown ([#1422](#1422)) ([7bc583b](7bc583b)) * sign out correctly on error ([#1452](#1452)) ([d08fc94](d08fc94)) * simplify usb listing ([#1402](#1402)) ([5355115](5355115)) * theme issues when sent from graph ([#1424](#1424)) ([75ad838](75ad838)) * **ui:** notifications positioning regression ([#1445](#1445)) ([f73e5e0](f73e5e0)) * use some instead of every for connect detection ([9ce2fee](9ce2fee)) ### Reverts * revert package.json dependency updates from commit 711cc9a for api and packages/* ([94420e4](94420e4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores