Rework settings #39
Rework settings #39
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes in this pull request introduce new classes for managing secrets across various integrations, enhancing the structure and validation of credentials using Pydantic's Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (48)
admyral/secret/secret.py (1)
4-11: 🛠️ Refactor suggestion
Add docstring and error handling.
The decorator implementation looks clean, but could benefit from documentation and error handling.
Consider applying these improvements:
def register_secret(secret_type: str) -> BaseModel: + """Register a secret model with the SecretRegistry. + + Args: + secret_type: Unique identifier for the secret type + + Returns: + A decorator that registers the decorated BaseModel class + + Example: + @register_secret("openai") + class OpenAISecret(BaseModel): + api_key: str + """ def inner(secret: BaseModel) -> BaseModel: from admyral.secret.secret_registry import SecretRegistry - SecretRegistry.register_secret(secret_type, secret) + try: + SecretRegistry.register_secret(secret_type, secret) + except Exception as e: + raise ValueError(f"Failed to register secret type '{secret_type}': {str(e)}") return secret return inner📝 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.def register_secret(secret_type: str) -> BaseModel: """Register a secret model with the SecretRegistry. Args: secret_type: Unique identifier for the secret type Returns: A decorator that registers the decorated BaseModel class Example: @register_secret("openai") class OpenAISecret(BaseModel): api_key: str """ def inner(secret: BaseModel) -> BaseModel: from admyral.secret.secret_registry import SecretRegistry try: SecretRegistry.register_secret(secret_type, secret) except Exception as e: raise ValueError(f"Failed to register secret type '{secret_type}': {str(e)}") return secret return innerweb/src/lib/utils.ts (1)
8-10: 🛠️ Refactor suggestion
Enhance validation and optimize performance.
Since this function is used for secret validation, consider these improvements:
- Use
some()instead offilter().lengthfor better performance- Add input validation
- Consider whitespace-only keys as empty
export function hasEmptyKey(secret: { key: string; value: string }[]): boolean { - return secret.filter((kv) => kv.key.length === 0).length > 0; + if (!Array.isArray(secret)) return true; + return secret.some((kv) => !kv?.key?.trim()); }📝 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.export function hasEmptyKey(secret: { key: string; value: string }[]): boolean { if (!Array.isArray(secret)) return true; return secret.some((kv) => !kv?.key?.trim()); }web/src/components/side-nav-settings/side-nav-settings.tsx (1)
4-25: 🛠️ Refactor suggestion
Consider refactoring repeated navigation items configuration.
The navigation items share similar structure and a common base path. Consider extracting this configuration to reduce repetition and improve maintainability:
+ const SETTINGS_NAV_ITEMS = [ + { href: '/settings', title: 'Account', selectionCriteria: ['/settings'] }, + { href: '/settings/secrets', title: 'Secrets', selectionCriteria: ['/settings/secrets'] }, + { href: '/settings/api-keys', title: 'API Keys', selectionCriteria: ['/settings/api-keys'] }, + ] as const; + - export const SideNavSettings: React.FC = () => ( + export const SideNavSettings: React.FC = () => ( <Box mt="4" width="165px"> - <SideNavItem - href="/settings" - title="Account" - selectionCriteria={["/settings"]} - basePath="/settings" - /> - <SideNavItem - href="/settings/secrets" - title="Secrets" - selectionCriteria={["/settings/secrets"]} - basePath="/settings" - /> - <SideNavItem - href="/settings/api-keys" - title="API Keys" - selectionCriteria={["/settings/api-keys"]} - basePath="/settings" - /> + {SETTINGS_NAV_ITEMS.map(({ href, title, selectionCriteria }) => ( + <SideNavItem + key={href} + href={href} + title={title} + selectionCriteria={selectionCriteria} + basePath="/settings" + /> + ))} </Box> );📝 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.const SETTINGS_NAV_ITEMS = [ { href: '/settings', title: 'Account', selectionCriteria: ['/settings'] }, { href: '/settings/secrets', title: 'Secrets', selectionCriteria: ['/settings/secrets'] }, { href: '/settings/api-keys', title: 'API Keys', selectionCriteria: ['/settings/api-keys'] }, ] as const; export const SideNavSettings: React.FC = () => ( <Box mt="4" width="165px"> {SETTINGS_NAV_ITEMS.map(({ href, title, selectionCriteria }) => ( <SideNavItem key={href} href={href} title={title} selectionCriteria={selectionCriteria} basePath="/settings" /> ))} </Box> );web/src/hooks/use-create-api-key.ts (2)
16-16:
⚠️ Potential issueConsider using
z.inferinstead ofz.inputfor the response type.Using
z.inputfor the API response type might lead to type-safety issues since it represents the shape of data before validation, rather than after. The API response should be typed with the validated shape usingz.infer.Apply this diff to fix the type:
- z.input<typeof ApiKey> + z.infer<typeof ApiKey>📝 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.z.infer<typeof ApiKey>
16-16: 💡 Codebase verification
Inconsistency found in API response type handling
The codebase search reveals that
use-create-api-key.tsis usingz.input<typeof ApiKey>for the response type while most other API hooks consistently usez.inferfor response types. Here's the pattern observed:
- Most hooks use
z.inputfor request types andz.inferfor response types- Only exceptions using different patterns:
use-create-api-key.ts: Usesz.inputfor both request and responseuse-get-workflow-api.tsanduse-list-credentials-api.ts: Usez.outputfor responseThe
use-create-api-key.tshook should be updated to usez.infer<typeof ApiKey>for consistency with the predominant pattern in the codebase.🔗 Analysis chain
Verify similar type definitions in other API hooks.
Let's check if this pattern exists in other API hooks to maintain consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find similar API type definitions in other hooks # Expected: Other hooks should use z.infer for response types # Search for API type definitions in hook files rg -l "api<" web/src/hooks/ # For each file found, show the type definition context rg -A 3 "api<" web/src/hooks/Length of output: 6249
web/src/hooks/use-delete-user-api.ts (1)
22-26: 🛠️ Refactor suggestion
Enhance hook with error handling and cache management.
The current implementation could benefit from:
- Error handling with proper typing
- Cache invalidation for related user queries
- Success/error callbacks for better UX
Consider this enhanced implementation:
export const useDeleteUserApi = () => { return useMutation({ mutationFn: () => deleteUserApi(), + onError: (error: Error) => { + console.error('Failed to delete user:', error); + }, + onSuccess: () => { + // Invalidate user-related queries + queryClient.invalidateQueries({ queryKey: ['user'] }); + } }); };Note: You'll need to configure and import the
queryClientfrom your React Query setup.Committable suggestion skipped: line range outside the PR's diff.
web/src/components/user-profile/user-profile.tsx (2)
14-16: 🛠️ Refactor suggestion
Pass error details to ErrorCallout for better error reporting.
The error state could be more informative by passing the actual error message to the ErrorCallout component.
if (error) { - return <ErrorCallout />; + return <ErrorCallout message={error instanceof Error ? error.message : 'Failed to load user profile'} />; }📝 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.if (error) { return <ErrorCallout message={error instanceof Error ? error.message : 'Failed to load user profile'} />; }
18-27: 🛠️ Refactor suggestion
Add type safety and improve accessibility.
The component needs TypeScript types for the user profile data and better accessibility support.
+interface UserProfile { + email: string; +} export default function UserProfile() { - const { data: userProfile, isPending, error } = useGetUserProfileApi(); + const { data: userProfile, isPending, error } = useGetUserProfileApi<UserProfile>(); // ... rest of the component return ( <Flex direction="column" width="296px"> - <label> + <label htmlFor="business-email"> <Text as="div" size="2" mb="1"> Business E-Mail </Text> - <TextField.Root value={userProfile.email} disabled /> + <TextField.Root + value={userProfile.email} + disabled + id="business-email" + aria-label="Business email address" + /> </label> </Flex> ); }Committable suggestion skipped: line range outside the PR's diff.
web/src/app/(workspace)/settings/api-keys/page.tsx (2)
6-25: 🛠️ Refactor suggestion
Consider adding error boundaries.
The component lacks error handling for potential failures in the
ApiKeysorCreateApiKeycomponents. Consider wrapping these components with error boundaries to gracefully handle runtime errors.Here's a suggested implementation:
import { ErrorBoundary } from '@/components/error-boundary'; export default function ApiKeysPage() { return ( <ErrorBoundary fallback={<Text>Failed to load API Keys section</Text>}> <Flex direction="column" justify="start" gap="4" p="5" width="90%"> <Flex align="start" justify="between" width="100%"> <Text size="4" weight="medium"> API Keys </Text> <CreateApiKey /> </Flex> <ApiKeys /> </Flex> </ErrorBoundary> ); }
8-8: 💡 Codebase verification
Feature flag implementation is inconsistent with the codebase pattern
The review comment's concern about feature flag implementation is valid, but needs correction:
- The codebase shows an established pattern of using
DISABLE_AUTHfeature flag across multiple files, particularly for authentication-related features.- In this file, the feature flag logic is commented out, breaking the consistent pattern seen in other files like
settings/(account)/page.tsx.- The current implementation always shows both
CreateApiKeyandApiKeyscomponents, ignoring the authentication state, which differs from the codebase's standard approach.The
width="90%"styling is consistently used across settings pages (api-keys,secrets, andaccount), so this isn't an issue.🔗 Analysis chain
Review the feature flag implementation strategy.
The commented-out conditional rendering logic suggests an incomplete feature flag implementation. This could lead to confusion about the intended behavior. Consider either:
- Implementing proper feature flag handling
- Removing the commented code if it's no longer needed
- Adding a TODO comment explaining the future plans
Also, the hardcoded
width="90%"might not provide the best responsive design. Consider using more flexible layout approaches.Let's check if there are other similar patterns in the codebase:
Also applies to: 14-22
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other instances of DISABLE_AUTH usage rg "DISABLE_AUTH" --type tsx --type ts # Check for similar width="90%" patterns that might need consistent styling rg 'width="90%"' --type tsxLength of output: 124
Script:
#!/bin/bash # Check for other instances of DISABLE_AUTH usage and commented code rg "DISABLE_AUTH" # Check for similar width="90%" patterns that might need consistent styling rg 'width="90%"' # Let's also check the specific file content to verify the commented code claim cat "web/src/app/(workspace)/settings/api-keys/page.tsx"Length of output: 3204
web/src/hooks/use-get-secret-schemas-api.ts (1)
22-27: 🛠️ Refactor suggestion
Enhance hook configuration and error handling.
The current implementation could benefit from additional configurations to improve caching, error handling, and retry behavior.
Consider implementing these improvements:
export const useGetSecretsSchemas = () => { return useQuery({ queryKey: ["secrets-schemas"], queryFn: () => getSecretsSchemasApi(), + staleTime: 5 * 60 * 1000, // Consider caching for 5 minutes + retry: (failureCount, error) => { + // Retry only for network errors, not for 4xx responses + return failureCount < 3 && error.status >= 500; + }, + select: (data) => { + // Transform the data if needed + return data; + }, }); };Also, consider adding JSDoc comments to document the hook's usage:
/** * Hook to fetch secret schemas from the API. * @returns {UseQueryResult} Query result containing schemas data, loading state, and error state * @example * const { data, isLoading, error } = useGetSecretsSchemas(); */web/src/app/(workspace)/settings/(account)/page.tsx (1)
18-28: 🛠️ Refactor suggestion
Improve auth buttons layout and conditional rendering.
The current implementation has several areas for improvement:
- Duplicate conditional checks can be consolidated
- Fixed width might not be responsive
- Buttons could be grouped for better visual hierarchy
Consider this more maintainable structure:
- {!DISABLE_AUTH && ( - <Box width="151px"> - <SignOutButton /> - </Box> - )} - - {!DISABLE_AUTH && ( - <Box width="151px"> - <DeleteAccountButton /> - </Box> - )} + {!DISABLE_AUTH && ( + <Flex direction="column" gap="2"> + <SignOutButton /> + <DeleteAccountButton /> + </Flex> + )}📝 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.{!DISABLE_AUTH && ( <Flex direction="column" gap="2"> <SignOutButton /> <DeleteAccountButton /> </Flex> )}admyral/db/alembic/versions/2f91aed0b218_add_secret_type_to_to_secrets_table.py (1)
23-26: 💡 Codebase verification
Based on the evidence gathered, I can now provide a final response:
Migration strategy is required for secret_type column
The codebase shows that
secret_typeis actively used in the application:
- It's validated against registered secret types via
SecretRegistry.is_registered()- There's a registry system that manages different secret types
- The field is used for schema validation and secret management
Recommendations:
- Add a data migration step to this migration to populate
secret_typefor existing secrets- Consider making the column NOT NULL after the data migration, since the application logic relies on this field
Example migration approach:
def upgrade() -> None: # Add the column first op.add_column("secrets", sa.Column("secret_type", sa.TEXT(), nullable=True)) # Add data migration here to set appropriate secret_type values # op.execute("UPDATE secrets SET secret_type = 'default_type' WHERE secret_type IS NULL") # Optionally make it NOT NULL after data migration # op.alter_column("secrets", "secret_type", nullable=False)🔗 Analysis chain
Consider data migration strategy for existing records
The migration adds a nullable
secret_typecolumn without a default value or data migration strategy. This raises several considerations:
- If
secret_typewill be required for new records, consider adding a NOT NULL constraint- Existing records will have NULL values - consider if a data migration is needed to backfill appropriate secret types
Let's verify if there are existing records in the secrets table that would need migration:
Would you like me to help draft a data migration strategy if needed?
Let me gather more specific information about how secrets are handled in the codebase.
Let me check one more aspect to understand if there's any default secret type being used or if the field is optional in the application logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for repository code that might indicate existing secrets usage # This will help determine if we need a data migration strategy # Look for existing secret types in the codebase rg -l 'secret.*type' --type py # Look for secret creation/insertion code ast-grep --pattern 'INSERT INTO secrets'Length of output: 1925
Script:
#!/bin/bash # Check secret model definition and usage rg -A 10 "class Secret" admyral/models/secret.py # Check secrets schema definition rg -A 10 "class Secret" admyral/db/schemas/secrets_schemas.py # Look for existing secret creation/storage logic rg -A 5 "def create_secret|def store_secret" --type py # Check for any existing secret type enums or constants rg "SecretType|SECRET_TYPE" --type pyLength of output: 1472
Script:
#!/bin/bash # Check for secret creation/validation logic in secret registry and manager rg -A 5 "def validate|def create" admyral/secret/secret_registry.py admyral/secret/secrets_manager.py # Look for any secret type validation or requirements rg -B 2 -A 2 "secret_type" admyral/secret/ # Check API endpoints or CLI commands that create secrets rg -A 5 "def create_secret" admyral/cli/secret.pyLength of output: 2696
admyral/secret/secret_registry.py (1)
15-20: 🛠️ Refactor suggestion
Add error handling and consider caching schema information.
The schema extraction could fail, and repeatedly generating schemas could be inefficient. Consider adding error handling and caching the schemas.
+ _schema_cache: dict[str, list[str]] = {} + @classmethod def get_secret_schemas(cls) -> dict[str, list[str]]: + if cls._schema_cache: + return cls._schema_cache.copy() + + schemas = {} + for secret_type, secret in cls._secrets.items(): + try: + schemas[secret_type] = list(secret.model_json_schema()["properties"].keys()) + except Exception as e: + raise ValueError(f"Failed to extract schema for {secret_type}: {str(e)}") + + cls._schema_cache = schemas + return schemas.copy() - return { - secret_type: secret.model_json_schema()["properties"].keys() - for secret_type, secret in cls._secrets.items() - }📝 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._schema_cache: dict[str, list[str]] = {} @classmethod def get_secret_schemas(cls) -> dict[str, list[str]]: if cls._schema_cache: return cls._schema_cache.copy() schemas = {} for secret_type, secret in cls._secrets.items(): try: schemas[secret_type] = list(secret.model_json_schema()["properties"].keys()) except Exception as e: raise ValueError(f"Failed to extract schema for {secret_type}: {str(e)}") cls._schema_cache = schemas return schemas.copy()web/src/app/(workspace)/settings/layout.tsx (2)
5-9: 🛠️ Refactor suggestion
Remove unnecessary async keyword.
The component doesn't perform any asynchronous operations, so the
asynckeyword is unnecessary and could impact performance by creating an unnecessary Promise wrapper.-export default async function SettingsPageLayout({ +export default function SettingsPageLayout({ children, }: { children: ReactNode; })📝 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.export default function SettingsPageLayout({ children, }: { children: ReactNode; }) {
35-42:
⚠️ Potential issueFix height calculation and improve responsiveness.
There are several issues in the content grid implementation:
- There's a typo in the height calculation (
calculateshould becalc)- The hardcoded column width of
165pxmight not work well on different screen sizesFirst, fix the critical typo:
<Grid columns="165px 1fr" width="auto" - height="calculate(100vh - 56px)" + height="calc(100vh - 56px)" >Consider improving the responsive design:
<Grid - columns="165px 1fr" + columns={{ + initial: "1fr", + sm: "165px 1fr" + }} width="auto" height="calc(100vh - 56px)" >📝 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.<Grid columns={{ initial: "1fr", sm: "165px 1fr" }} width="auto" height="calc(100vh - 56px)" > <SideNavSettings /> <Box>{children}</Box> </Grid>web/src/components/utils/secret-text-field.tsx (2)
7-11: 🛠️ Refactor suggestion
Consider strengthening prop types for better type safety.
The current prop types could be improved:
valueandonChangeshould be required props to ensure controlled component behaviorvalueshould be restricted to string type as numbers might cause unexpected behavior with TextFieldinterface SecretTextFieldProps { - value?: string | number | undefined; + value: string; placeholder?: string | undefined; - onChange?: ChangeEventHandler<HTMLInputElement> | undefined; + onChange: ChangeEventHandler<HTMLInputElement>; }📝 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.interface SecretTextFieldProps { value: string; placeholder?: string | undefined; onChange: ChangeEventHandler<HTMLInputElement>; }
27-34: 🛠️ Refactor suggestion
Enhance security and improve button implementation.
Consider these improvements:
- Use Radix's built-in cursor prop instead of inline styles
- Add aria-label to the toggle button
- Consider auto-hiding the value on blur for additional security
<IconButton size="1" variant="ghost" onClick={() => setShowValue((v) => !v)} - style={{ cursor: "pointer" }} + cursor="pointer" + aria-label={showValue ? "Hide secret value" : "Show secret value"} + onBlur={() => setShowValue(false)} > {showValue ? <EyeOpenIcon /> : <EyeClosedIcon />} </IconButton>📝 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.<IconButton size="1" variant="ghost" onClick={() => setShowValue((v) => !v)} cursor="pointer" aria-label={showValue ? "Hide secret value" : "Show secret value"} onBlur={() => setShowValue(false)} > {showValue ? <EyeOpenIcon /> : <EyeClosedIcon />} </IconButton>web/src/hooks/use-update-secret-api.ts (1)
29-45: 🛠️ Refactor suggestion
Improve type safety and performance of secret transformation.
The current implementation has several areas for improvement:
- The reduce operation lacks type safety
- No handling for duplicate keys
- Missing validation for empty secrets array
- Could be optimized using Object.fromEntries
export const useUpdateSecretApi = () => { return useMutation({ mutationFn: ({ secretId, secret, secretType }: TSecret) => { + if (secret.length === 0) { + throw new Error("Secret array cannot be empty"); + } + + // Check for duplicate keys + const keys = new Set(); + for (const { key } of secret) { + if (keys.has(key)) { + throw new Error(`Duplicate key found: ${key}`); + } + keys.add(key); + } + return updateSecret({ secretId, - secret: secret.reduce( - (prev, secret) => { - prev[secret.key] = secret.value; - return prev; - }, - {} as Record<string, string>, - ), + secret: Object.fromEntries( + secret.map(({ key, value }) => [key, value]) + ), secretType, }); }, }); };📝 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.export const useUpdateSecretApi = () => { return useMutation({ mutationFn: ({ secretId, secret, secretType }: TSecret) => { if (secret.length === 0) { throw new Error("Secret array cannot be empty"); } // Check for duplicate keys const keys = new Set(); for (const { key } of secret) { if (keys.has(key)) { throw new Error(`Duplicate key found: ${key}`); } keys.add(key); } return updateSecret({ secretId, secret: Object.fromEntries( secret.map(({ key, value }) => [key, value]) ), secretType, }); }, }); };web/src/stores/secrets-store.ts (1)
27-32: 🛠️ Refactor suggestion
Consider validating newSecret before adding.
The
addNewSecretmethod accepts a secret without validation. Consider adding runtime validation to ensure the secret meets all requirements before adding it to the store.Example implementation:
addNewSecret: (newSecret: TSecretMetadata) => set( produce((draft) => { + if (!newSecret.secretId) { + throw new Error('Secret must have a secretId'); + } + if (draft.secrets.some(s => s.secretId === newSecret.secretId)) { + throw new Error('Secret with this ID already exists'); + } draft.secrets.unshift(newSecret); }), ),Committable suggestion skipped: line range outside the PR's diff.
admyral/actions/integrations/enrich/greynoise.py (2)
16-23: 🛠️ Refactor suggestion
Add timeout and make base URL configurable.
The HTTP client configuration could be improved for better reliability and testability:
- Add a timeout to prevent hanging requests
- Consider making the base URL configurable for testing purposes
def get_grey_noise_client(secret: GreyNoiseSecret) -> Client: return Client( base_url="https://api.greynoise.io/v3", + timeout=30.0, headers={ "x-apikey": secret.api_key, "Content-Type": "application/json", "Accept": "application/json", }, )Committable suggestion skipped: line range outside the PR's diff.
43-45: 🛠️ Refactor suggestion
Improve error handling for secret validation.
The secret validation could fail if the secret is missing or invalid. Consider adding explicit error handling:
secret = ctx.get().secrets.get("GREY_NOISE_SECRET") + if not secret: + raise ValueError("GreyNoise API key not configured") secret = GreyNoiseSecret.model_validate(secret) with get_grey_noise_client(secret) as client:📝 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.secret = ctx.get().secrets.get("GREY_NOISE_SECRET") if not secret: raise ValueError("GreyNoise API key not configured") secret = GreyNoiseSecret.model_validate(secret) with get_grey_noise_client(secret) as client:web/src/components/secrets/secrets.tsx (1)
47-49:
⚠️ Potential issueAdd missing key prop to mapped elements.
React requires a unique key prop for elements in an array to properly handle updates and re-renders.
Apply this diff to fix the issue:
- <SecretRow secret={secret} idx={idx} /> + <SecretRow key={secret.id ?? idx} secret={secret} idx={idx} />📝 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.{secrets.map((secret, idx) => ( <SecretRow key={secret.id ?? idx} secret={secret} idx={idx} /> ))}🧰 Tools
🪛 Biome
[error] 48-48: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
admyral/server/endpoints/secret_endpoints.py (1)
21-26: 💡 Codebase verification
⚠️ Potential issueRemove unnecessary and potentially unsafe encoding transformation
The
encode("utf-8").decode("unicode_escape")pattern is unique tosecret_endpoints.pyand isn't used anywhere else in the codebase for secret handling. Other parts of the codebase handle encoding/decoding in standard ways (base64, UTF-8) for specific purposes like API authentication or file operations.The current implementation:
- Is unnecessary since secret values can be stored as strings directly
- Could introduce security vulnerabilities by interpreting escape sequences
- Isn't consistent with how secrets are handled in the rest of the codebase
Recommendation:
secret.secret = {key: str(val) for key, val in secret.secret.items()}🔗 Analysis chain
Review the secret value encoding implementation.
The current encoding/decoding pattern raises several concerns:
- The
encode("utf-8").decode("unicode_escape")sequence seems unnecessary and could potentially lead to security vulnerabilities if malicious escape sequences are provided- This transformation might corrupt valid secret values that contain backslashes or unicode characters
Consider simplifying to:
- secret.secret = dict( - [ - (key, val.encode("utf-8").decode("unicode_escape")) - for key, val in secret.secret.items() - ] - ) + secret.secret = {key: str(val) for key, val in secret.secret.items()}Let's check if this encoding pattern is used elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar encoding patterns that might need fixing rg "encode.*decode.*unicode_escape" -g '!secret_endpoints.py'Length of output: 63
Script:
#!/bin/bash # Let's first check if there are any other files handling secrets or encoding rg -l "secret" --type python # Then check for any unicode or encoding related patterns rg "encode|decode|unicode" --type pythonLength of output: 133
Script:
#!/bin/bash # Search for Python files handling secrets fd '.*\.py$' | xargs rg -l "secret" # Check for encoding/decoding patterns in Python files fd '.*\.py$' | xargs rg "encode|decode|unicode"Length of output: 7188
admyral/actions/integrations/enrich/abuseipdb.py (1)
58-60: 🛠️ Refactor suggestion
Consider adding explicit error handling for secret validation.
The secret validation could fail if the data structure is incorrect. Consider adding explicit error handling to provide a more user-friendly error message.
- secret = ctx.get().secrets.get("ABUSEIPDB_SECRET") - secret = AbuseIPDBSecret.model_validate(secret) + raw_secret = ctx.get().secrets.get("ABUSEIPDB_SECRET") + if not raw_secret: + raise ValueError("AbuseIPDB secret is not configured") + try: + secret = AbuseIPDBSecret.model_validate(raw_secret) + except ValidationError as e: + raise ValueError(f"Invalid AbuseIPDB secret configuration: {e}")Committable suggestion skipped: line range outside the PR's diff.
admyral/actions/integrations/edr/sentinel_one.py (1)
60-60: 💡 Codebase verification
Validation error handling needs to be added
The code lacks error handling for Pydantic validation failures. Add a try-except block to handle
ValidationErrorwhen validating the SentinelOne secret:try: secret = SentinelOneSecret.model_validate(secret) except ValidationError as e: raise ConfigurationError("Invalid SentinelOne secret configuration") from e
admyral/actions/integrations/edr/sentinel_one.py:60🔗 Analysis chain
LGTM! Verify secret validation handling.
The changes properly integrate the new secret model validation.
Let's verify the error handling for invalid secrets:
Also applies to: 67-67
The previous regex pattern was too restrictive. Let's broaden our search to understand the validation context better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for try-except blocks around model_validate calls rg -U "try.*?\bmodel_validate\b.*?except.*?}" --multilineLength of output: 59
Script:
#!/bin/bash # Let's search for the SentinelOneSecret model definition and its usage ast-grep --pattern 'class SentinelOneSecret' # Also search for any error handling around this specific secret validation rg -B 2 -A 2 "SentinelOneSecret.model_validate"Length of output: 468
Script:
#!/bin/bash # Let's look for the base secret model and any validation error handling patterns rg -B 3 -A 3 "class.*Secret.*BaseModel" # Also check for any validation error handling in the module rg -B 2 -A 4 "ValidationError|pydantic\.error" admyral/actions/integrations/edr/sentinel_one.pyLength of output: 13422
Script:
#!/bin/bash # Let's check for any error handling in the context where secrets are used rg -B 3 -A 3 "try.*?get\(\"SENTINEL_ONE_SECRET\"\)" --multiline # Also check the general error handling pattern in the module rg -B 2 -A 2 "except.*?Exception" admyral/actions/integrations/edr/sentinel_one.pyLength of output: 148
admyral/actions/integrations/email/abnormal_security.py (1)
58-62: 🛠️ Refactor suggestion
Add error handling for secret validation.
While the secret validation is correctly implemented, consider adding explicit error handling for validation failures.
- secret = AbnormalSecuritySecret.model_validate(secret) + try: + secret = AbnormalSecuritySecret.model_validate(secret) + except ValidationError as e: + raise ValueError(f"Invalid Abnormal Security secret configuration: {e}")📝 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.try: secret = AbnormalSecuritySecret.model_validate(secret) except ValidationError as e: raise ValueError(f"Invalid Abnormal Security secret configuration: {e}") filter = f"createdTime gte {start_time} lte {end_time}" with get_abnormal_security_client(secret) as client:admyral/actions/integrations/ai/mistralai.py (1)
67-70:
⚠️ Potential issueAdd error handling for secret validation and API calls.
The current implementation lacks proper error handling:
- No try-catch for secret validation
- No handling of API client initialization errors
- No handling of API call failures
Consider implementing comprehensive error handling:
- secret = ctx.get().secrets.get("MISTRALAI_SECRET") - secret = MistralAISecret.model_validate(secret) - - client = MistralClient(api_key=secret.api_key) + try: + secret_dict = ctx.get().secrets.get("MISTRALAI_SECRET") + if not secret_dict: + raise ValueError("Mistral AI secret not found") + + secret = MistralAISecret.model_validate(secret_dict) + client = MistralClient(api_key=secret.api_key) + except ValidationError as e: + raise ValueError(f"Invalid Mistral AI secret configuration: {str(e)}") from e + except Exception as e: + raise RuntimeError(f"Failed to initialize Mistral AI client: {str(e)}") from e📝 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.try: secret_dict = ctx.get().secrets.get("MISTRALAI_SECRET") if not secret_dict: raise ValueError("Mistral AI secret not found") secret = MistralAISecret.model_validate(secret_dict) client = MistralClient(api_key=secret.api_key) except ValidationError as e: raise ValueError(f"Invalid Mistral AI secret configuration: {str(e)}") from e except Exception as e: raise RuntimeError(f"Failed to initialize Mistral AI client: {str(e)}") from eadmyral/actions/integrations/ai/openai.py (1)
68-68:
⚠️ Potential issueImplement comprehensive error handling as noted in TODO.
The function should handle common OpenAI API errors (rate limits, invalid API key, etc.) and provide meaningful error messages without exposing sensitive information.
Would you like me to help implement proper error handling for this function? Here's what we should cover:
- Authentication errors
- Rate limiting
- Invalid model selection
- API timeouts
- Network issues
admyral/actions/integrations/cases/pagerduty.py (1)
17-25: 🛠️ Refactor suggestion
Add timeout and error handling to client initialization.
The client initialization should include:
- Request timeout to prevent hanging
- Error handling for invalid credentials
def get_pagerduty_client(secret: PagerDutySecret) -> Client: + # Verify credentials before returning client + test_client = Client( + base_url="https://api.pagerduty.com", + headers={ + "Content-Type": "application/json", + "Accept": "application/json", + "Authorization": f"Token token={secret.api_key}", + "From": secret.email, + }, + timeout=10.0 + ) + try: + # Test authentication with a lightweight API call + response = test_client.get("/users/me") + response.raise_for_status() + except Exception as e: + raise ValueError("Invalid PagerDuty credentials") from e + finally: + test_client.close() + return Client( base_url="https://api.pagerduty.com", headers={ "Content-Type": "application/json", "Accept": "application/json", "Authorization": f"Token token={secret.api_key}", "From": secret.email, - }, + }, + timeout=10.0 )📝 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.def get_pagerduty_client(secret: PagerDutySecret) -> Client: # Verify credentials before returning client test_client = Client( base_url="https://api.pagerduty.com", headers={ "Content-Type": "application/json", "Accept": "application/json", "Authorization": f"Token token={secret.api_key}", "From": secret.email, }, timeout=10.0 ) try: # Test authentication with a lightweight API call response = test_client.get("/users/me") response.raise_for_status() except Exception as e: raise ValueError("Invalid PagerDuty credentials") from e finally: test_client.close() return Client( base_url="https://api.pagerduty.com", headers={ "Content-Type": "application/json", "Accept": "application/json", "Authorization": f"Token token={secret.api_key}", "From": secret.email, }, timeout=10.0 )admyral/actions/integrations/enrich/leakcheck.py (1)
68-73: 🛠️ Refactor suggestion
Add null check for secret.
While the secret validation is good, consider adding a null check to handle cases where the secret is not found in the context.
secret = ctx.get().secrets.get("LEAKCHECK_SECRET") + if not secret: + raise NonRetryableActionError("LeakCheck API key not found in secrets") secret = LeakCheckSecret.model_validate(secret)Committable suggestion skipped: line range outside the PR's diff.
admyral/actions/integrations/compliance/zendesk.py (1)
19-26: 🛠️ Refactor suggestion
Add error handling for connection issues.
The client initialization should handle potential connection errors and invalid credentials gracefully.
def get_zendesk_client(secret: ZendeskSecret) -> Client: + try: return Client( base_url=f"https://{secret.subdomain}.zendesk.com/api", headers={ "Content-Type": "application/json", }, auth=(f"{secret.email}/token", secret.api_token), ) + except Exception as e: + raise NonRetryableActionError(f"Failed to initialize Zendesk client: {str(e)}")📝 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.def get_zendesk_client(secret: ZendeskSecret) -> Client: try: return Client( base_url=f"https://{secret.subdomain}.zendesk.com/api", headers={ "Content-Type": "application/json", }, auth=(f"{secret.email}/token", secret.api_token), ) except Exception as e: raise NonRetryableActionError(f"Failed to initialize Zendesk client: {str(e)}")admyral/actions/integrations/compliance/one_password.py (1)
17-24: 🛠️ Refactor suggestion
Add error handling for connection issues.
The client creation could fail due to network issues or invalid credentials. Consider adding proper error handling.
def get_1password_client(secret: OnePasswordSecret) -> Client: + try: return Client( base_url=f"https://{secret.domain}", headers={ "Authorization": f"Bearer {secret.api_key}", "Content-Type": "application/json", "Accept": "application/json", }, ) + except Exception as e: + raise ValueError(f"Failed to create 1Password client: {str(e)}") from e📝 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.def get_1password_client(secret: OnePasswordSecret) -> Client: try: return Client( base_url=f"https://{secret.domain}", headers={ "Authorization": f"Bearer {secret.api_key}", "Content-Type": "application/json", "Accept": "application/json", }, ) except Exception as e: raise ValueError(f"Failed to create 1Password client: {str(e)}") from eadmyral/actions/integrations/cdr/wiz.py (1)
174-176: 🛠️ Refactor suggestion
Add explicit error handling for missing or invalid secrets.
While the validation is good, consider handling potential errors more explicitly:
secret = ctx.get().secrets.get("WIZ_SECRET") + if not secret: + raise ValueError("WIZ_SECRET not found in context") + + try: secret = WizSecret.model_validate(secret) + except ValidationError as e: + raise ValueError(f"Invalid WIZ_SECRET configuration: {e}") with get_wiz_client(secret) as client:Committable suggestion skipped: line range outside the PR's diff.
admyral/actions/integrations/cases/opsgenie.py (1)
136-136: 🛠️ Refactor suggestion
Add error handling for missing or invalid secrets.
While the validation using
model_validateis correct, consider adding explicit error handling for missing or invalid secrets.opsgenie_secret = ctx.get().secrets.get("OPSGENIE_SECRET") + if not opsgenie_secret: + raise ValueError("OpsGenie secret not found in context") + try: opsgenie_secret = OpsGenieSecret.model_validate(opsgenie_secret) + except ValueError as e: + raise ValueError(f"Invalid OpsGenie secret configuration: {e}")Committable suggestion skipped: line range outside the PR's diff.
admyral/actions/integrations/cases/jira.py (1)
126-126: 🛠️ Refactor suggestion
Consider adding explicit error handling for secret validation.
The
model_validatecalls could fail if the secret data is malformed. Consider wrapping these in try-except blocks with meaningful error messages:- secret = JiraSecret.model_validate(secret) + try: + secret = JiraSecret.model_validate(secret) + except ValidationError as e: + raise ValueError(f"Invalid Jira credentials: {e}") from eAlso applies to: 194-194, 232-232, 266-266, 326-326
admyral/actions/integrations/cloud/aws.py (1)
60-63: 🛠️ Refactor suggestion
Consider reducing credential handling duplication.
Multiple functions share identical credential handling logic. Consider extracting this into a helper function to improve maintainability and reduce duplication.
+def get_aws_credentials() -> AWSSecret: + """Get validated AWS credentials from the context.""" + try: + secret = ctx.get().secrets.get("AWS_SECRET") + return AWSSecret.model_validate(secret) + except ValueError as e: + raise ValueError(f"Invalid AWS credentials configuration: {e}") from e def aws_s3_bucket_logging_enabled(...): - secret = ctx.get().secrets.get("AWS_SECRET") - secret = AWSSecret.model_validate(secret) + secret = get_aws_credentials() return run_steampipe_query( query, secret.aws_access_key_id, secret.aws_secret_access_key )Also applies to: 114-117, 189-192, 268-271, 302-305
admyral/db/admyral_store.py (1)
151-155:
⚠️ Potential issueConsider handling cascading deletions for user-related data.
The
delete_usermethod should handle the cleanup of related data (API keys, secrets, workflows, etc.) to maintain data integrity. Consider implementing this either through database CASCADE constraints or by explicitly deleting related records.web/src/components/api-keys/api-keys.tsx (1)
74-74: 🛠️ Refactor suggestion
Add a unique
keyprop toTable.Rowin theapiKeys.map()Each child in a list should have a unique
keyprop to help React efficiently update and render components.Apply this diff to add the
keyprop:- <Table.Row> + <Table.Row key={apiKey.id}>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 74-74: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
admyral/actions/integrations/ai/azure_openai.py (2)
10-14:
⚠️ Potential issueCritical Issue: Mismatch between
AzureOpenAISecretattributes and usageThe
AzureOpenAISecretclass definesdeployment_name, but the code accessessecret.modelat lines 75 and 80. This will result in anAttributeErrorat runtime sincemodelis not an attribute ofAzureOpenAISecret. Consider updating the class or adjusting the code to ensure consistency.Apply one of the following fixes:
Option 1: Update the secret class to include
modelclass AzureOpenAISecret(BaseModel): endpoint: str api_key: str + model: strOption 2: Modify the code to use
secret.deployment_name- if stop_tokens is not None and not secret.model.startswith("o1"): + if stop_tokens is not None and not secret.deployment_name.startswith("o1"): model_params["stop"] = stop_tokens chat_completion = client.chat.completions.create( messages=[{"role": "user", "content": prompt}], - model=secret.model, + model=secret.deployment_name, **model_params, )Also applies to: 75-81
2-2:
⚠️ Potential issueCritical Issue: Incorrect usage of
AzureOpenAIclass fromopenaipackageThe
openaipackage does not provide anAzureOpenAIclass. Instead, you should configure theopenaiclient for Azure OpenAI services. This code will raise anImportErrororAttributeErrorat runtime.Apply the following changes to fix the issue:
Import the
openaipackage-from openai import AzureOpenAI +import openaiConfigure the
openaiclient for Azure OpenAIsecret = ctx.get().secrets.get("AZURE_OPENAI_SECRET") secret = AzureOpenAISecret.model_validate(secret) -client = AzureOpenAI( - api_version="2024-06-01", azure_endpoint=secret.endpoint, api_key=secret.api_key -) +openai.api_type = "azure" +openai.api_base = secret.endpoint +openai.api_key = secret.api_key +openai.api_version = "2024-06-01"Modify the chat completion call
-chat_completion = client.chat.completions.create( +chat_completion = openai.ChatCompletion.create( messages=[{"role": "user", "content": prompt}], - model=secret.model, + engine=secret.deployment_name, **model_params, )Note: When interfacing with Azure OpenAI, use
engine=secret.deployment_nameinstead ofmodel.Also applies to: 64-67
admyral/secret/secrets_manager.py (1)
87-105: 🛠️ Refactor suggestion
Review the logic for updating and removing secret fields
In the
updatemethod, the current logic performs the following:
- Updates existing secret fields with values from
delta_secret, skipping fields with empty values.- If the
delta_secret.secret_typeis not registered, it removes keys fromsecret.secretthat are not present indelta_secret.secret.Consider the following points for clarity and correctness:
- Empty Values Handling: Should fields in
delta_secretwith empty values clear the corresponding fields insecret.secretinstead of being ignored? Currently, they are skipped, which might not reflect an intentional field clearing.- Secret Type Registration Check: Is the condition
if not SecretRegistry.is_registered(delta_secret.secret_type):appropriate? If the secret type is not registered, the method removes keys. Verify if this behavior aligns with the intended functionality. Should the removal of keys occur when the secret type is registered instead?To improve the code, consider adjusting the logic or adding comments to explain the intended behavior. For example, if the intention is to allow dynamic schema editing for unregistered secret types, clarify this in comments.
web/src/components/secrets/secret-row.tsx (3)
234-234:
⚠️ Potential issueAdd a unique
keyprop to elements in list rendering.When rendering a list using
map, each element should have a uniquekeyprop to help React identify which items have changed or need to be re-rendered.Apply this diff to fix the issue:
<label> + <label key={idx}>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 234-234: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
260-260:
⚠️ Potential issueAdd a unique
keyprop to elements in list rendering.Similarly, the
<Flex>component rendered inside themapfunction should have a uniquekeyprop.Apply this diff to fix the issue:
<Flex direction="column"> + <Flex direction="column" key={idx}>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 260-260: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
75-114:
⚠️ Potential issueHandle errors in the
handleUpdateSecretfunction.In the
handleUpdateSecretfunction, errors fromupdateSecretApi.mutateAsyncare not being caught. This could lead to unhandled promise rejections and a poor user experience if the update fails without feedback.Apply this diff to add error handling:
const handleUpdateSecret = async () => { try { setIsUpdating(true); setDialogState((draft) => { draft.error = null; }); if (hasEmptyKey(dialogState.secret)) { setDialogState((draft) => { draft.error = "Keys must not be empty."; }); return; } if ( hasNonEmptyValueFieldsForNewFields( dialogState.secret, dialogState.isNew, ) ) { setDialogState((draft) => { draft.error = "Values must not be empty for newly added fields."; }); return; } + const updatedSecret = await updateSecretApi + .mutateAsync({ + ...secret, + secret: dialogState.secret, + }) + .catch((error) => { + setDialogState((draft) => { + draft.error = "Failed to update secret. Please try again."; + }); + throw error; // Re-throw to ensure `finally` block executes properly + }); updateSecret(idx, updatedSecret); setDialogState((draft) => { draft.open = false; }); } finally { setIsUpdating(false); } };📝 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.const handleUpdateSecret = async () => { try { setIsUpdating(true); setDialogState((draft) => { draft.error = null; }); if (hasEmptyKey(dialogState.secret)) { setDialogState((draft) => { draft.error = "Keys must not be empty."; }); return; } if ( hasNonEmptyValueFieldsForNewFields( dialogState.secret, dialogState.isNew, ) ) { setDialogState((draft) => { draft.error = "Values must not be empty for newly added fields."; }); return; } const updatedSecret = await updateSecretApi .mutateAsync({ ...secret, secret: dialogState.secret, }) .catch((error) => { setDialogState((draft) => { draft.error = "Failed to update secret. Please try again."; }); throw error; // Re-throw to ensure `finally` block executes properly }); updateSecret(idx, updatedSecret); setDialogState((draft) => { draft.open = false; }); } finally { setIsUpdating(false); } };web/src/components/secrets/add-secret.tsx (3)
117-135:
⚠️ Potential issueAdd unique
keyprop to list items inmapfunctionThe
DropdownMenu.Itemcomponents generated by themapfunction should include a uniquekeyprop. This helps React identify which items have changed and optimize rendering.Apply this diff to fix the issue:
116-139 {secretsSchemas && Object.keys(secretsSchemas).map((secretType) => ( + <DropdownMenu.Item + key={secretType} style={{ cursor: "pointer" }} onClick={() => { if (secretsSchemas) { setDialogState({ open: true, secretId: "", secretType, secret: secretsSchemas![secretType].map((field) => ({ key: field, value: "", })), error: null, }); } }} > <NamespaceIcon namespace={secretType} /> {secretType} </DropdownMenu.Item> ))}📝 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.<DropdownMenu.Item key={secretType} style={{ cursor: "pointer" }} onClick={() => { if (secretsSchemas) { setDialogState({ open: true, secretId: "", secretType, secret: secretsSchemas![ secretType ].map((field) => ({ key: field, value: "", })), error: null, }); } }} >🧰 Tools
🪛 Biome
[error] 117-135: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
261-312:
⚠️ Potential issueAdd unique
keyprop to list items inmapfunctionThe
<Flex>components generated by thedialogState.secret.mapshould include a uniquekeyprop. This ensures efficient rendering and proper state management.Apply this diff to fix the issue:
260-312 {dialogState.secret.map((keyValue, idx) => ( + <Flex key={idx} direction="column"> <Flex justify="between" mb="1"> <Text as="div" size="2" mb="1" weight="bold" > Enter Key and Value </Text> <IconButton variant="soft" color="red" size="1" style={{ cursor: "pointer", }} onClick={() => setDialogState((draft) => { draft.secret.splice(idx, 1); }) } > <MinusIcon /> </IconButton> </Flex> <TextField.Root mb="2" value={keyValue.key} onChange={(event) => setDialogState((draft) => { draft.secret[idx].key = event.target.value; }) } placeholder="Key" /> <SecretTextField value={keyValue.value} onChange={(event) => setDialogState((draft) => { draft.secret[idx].value = event.target.value; }) } placeholder="Value" /> </Flex> ))}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 261-261: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
235-257:
⚠️ Potential issueAdd unique
keyprop to list items inmapfunctionThe
<label>elements generated by thedialogState.secret.mapshould include a uniquekeyprop to help React track item changes.Apply this diff to fix the issue:
234-258 {dialogState.secret.map((keyValue, idx) => ( + <label key={idx}> <Text as="div" size="2" mb="1" weight="bold" > {snakeCaseToCapitalizedCase(keyValue.key)} </Text> <SecretTextField value={keyValue.value} onChange={(event) => setDialogState((draft) => { draft.secret[idx].value = event.target.value; }) } placeholder="Your Value" /> </label> ))}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 235-235: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (39)
web/src/app/(workspace)/settings/api-keys/page.tsx (3)
8-15: Consider using a consistent layout container.The hardcoded width of 90% might not be consistent with other pages in the application. Consider using a shared layout container component or a design system constant for consistent spacing across pages.
17-20: Enhance the disabled auth message and add error boundaries.
- The message for disabled auth could be more informative and styled consistently.
- Consider wrapping child components with error boundaries to gracefully handle potential runtime errors.
+import { ErrorBoundary } from '@/components/error-boundary'; + export default function ApiKeysPage() { return ( <Flex direction="column" justify="start" gap="4" p="5" width="90%"> {/* ... header ... */} - {!DISABLE_AUTH && <ApiKeys />} + {!DISABLE_AUTH && ( + <ErrorBoundary> + <ApiKeys /> + </ErrorBoundary> + )} {DISABLE_AUTH && ( - <Text>API Keys are not available for local hosting.</Text> + <Text color="gray"> + API Keys are disabled in local development mode. Deploy to production to access this feature. + </Text> )} </Flex> ); }
6-23: Add TypeScript type annotations.Consider adding TypeScript type annotations to improve code maintainability and catch potential type-related issues early.
-export default function ApiKeysPage() { +export default function ApiKeysPage(): React.ReactElement {web/src/hooks/use-set-secret-api.ts (1)
31-35: LGTM: Clean implementation with potential for optimization.The new implementation is type-safe and explicit. The use of
Object.fromEntrieswithmapis clean, but could be optimized for large secret arrays.Consider using
reducefor better performance with large arrays:-secret: Object.fromEntries(secret.map((s) => [s.key, s.value])), +secret: secret.reduce((acc, s) => ({ ...acc, [s.key]: s.value }), {}),web/src/components/side-nav-settings/side-nav-item.tsx (3)
1-13: Add JSDoc comments to document the props interface.The code structure and imports look good. Consider adding JSDoc comments to document the purpose and expected values of each prop in the
SideNavItemPropsinterface.Add documentation like this:
+/** + * Props for the SideNavItem component + * @interface SideNavItemProps + * @property {string} href - The target URL for the navigation item + * @property {string} title - The display text for the navigation item + * @property {string[]} selectionCriteria - Array of paths that should trigger the selected state + * @property {string} basePath - The base path used for selection comparison + */ interface SideNavItemProps { href: string; title: string; selectionCriteria: string[]; basePath: string; }
21-26: Refactor selection logic for better readability.The selection logic is complex and could be more readable. Consider extracting it into a separate function with descriptive variable names.
+const isBasePath = (criteria: string, path: string, base: string) => + criteria === base && path === criteria; + +const isSubPath = (criteria: string, path: string, base: string) => + criteria !== base && path.startsWith(criteria); + export default function SideNavItem({ title, href, selectionCriteria, basePath, }: SideNavItemProps) { const pathname = usePathname(); - const isSelected = selectionCriteria.some( - (criteria) => - (criteria === basePath && pathname === criteria) || - (criteria !== basePath && pathname.startsWith(criteria)), + const isSelected = selectionCriteria.some((criteria) => + isBasePath(criteria, pathname, basePath) || isSubPath(criteria, pathname, basePath) );
31-47: Add aria-current for better accessibility.The component structure looks good, but could benefit from adding the
aria-currentattribute for better accessibility when the item is selected.<Link href={href}> <Flex width="135px" align="center" className={cn(defaultClassName, activateBackground)} mx="2" mt="1" px="2" py="2" + aria-current={isSelected ? "page" : undefined} > <Text size="2" weight={isSelected ? "bold" : "medium"}> {title} </Text> </Flex> </Link>web/src/components/publish-workflow-toggle/publish-workflow-toggle-base.tsx (1)
30-30: LGTM! AddinginfoToastto the dependency array is correct.The addition of
infoToastto theuseEffectdependency array follows React's hooks best practices and prevents potential stale closure issues.Consider adding error handling for failed workflow toggles. Here's a suggested implementation:
useEffect(() => { if (publishWorkflow.isSuccess && isLive !== publishWorkflow.data) { updateIsLiveState(publishWorkflow.data); publishWorkflow.reset(); infoToast( publishWorkflow.data ? "Workflow is activated." : "Workflow is deactivated.", ); } + if (publishWorkflow.isError) { + errorToast( + `Failed to ${isLive ? 'deactivate' : 'activate'} workflow: ${publishWorkflow.error.message}` + ); + publishWorkflow.reset(); + } }, [publishWorkflow, isLive, updateIsLiveState, infoToast]);web/src/components/navbar/navlink.tsx (1)
21-25: LGTM! The selection logic is more efficient now.The change from
filter().lengthtosome()is a good optimization as it short-circuits on the first match. The logic correctly handles both root path and nested paths.Consider adding type safety to the criteria comparison:
const isSelected = selectionCriteria.some( (criteria) => - (criteria === "/" && pathname === criteria) || - (criteria !== "/" && pathname.startsWith(criteria)), + (criteria === "/" && pathname === "/") || + (criteria !== "/" && typeof pathname === "string" && pathname.startsWith(criteria)), );web/src/components/secrets/secrets.tsx (2)
Line range hint
19-33: Consider adding a loading skeleton for better UX.Returning
nullduring loading might cause layout shifts. Consider using a table skeleton loader to maintain consistent layout and improve user experience.if (isListingSecretsLoading) { - return null; + return ( + <Table.Root> + <Table.Header> + {/* Same header structure */} + </Table.Header> + <Table.Body> + {Array(3).fill(0).map((_, idx) => ( + <Table.Row key={idx}> + <Table.Cell><div className="h-4 bg-gray-200 animate-pulse rounded" /></Table.Cell> + {/* Repeat for other cells */} + </Table.Row> + ))} + </Table.Body> + </Table.Root> + ); }
47-53: Remove index from SecretRow key prop.Since you already have a unique
secretId, the index in the key prop is unnecessary and could potentially cause issues with React's reconciliation if the array order changes.-key={`secret_row_${secret.secretId}`} +key={secret.secretId}admyral/server/endpoints/secret_endpoints.py (2)
26-31: Improved endpoint with better status code and return typeThe changes enhance the endpoint by:
- Using 201 CREATED (more appropriate than 204 NO CONTENT)
- Returning SecretMetadata (provides valuable feedback)
- Processing secrets consistently through handle_secret_values
Consider defining the Depends injection at module level to address B008:
authenticate_dependency = Depends(authenticate)🧰 Tools
🪛 Ruff
28-28: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
59-63: Consider caching schema responseSince secret schemas are likely static and this endpoint requires computation to gather all registered schemas, consider caching the response:
+from functools import lru_cache + +@lru_cache(maxsize=1) +def _get_cached_schemas() -> dict[str, list[str]]: + return SecretRegistry.get_secret_schemas() + @router.get("/schemas") async def get_secret_schemas( _authenticated_user: AuthenticatedUser = Depends(authenticate), ) -> dict[str, list[str]]: - return SecretRegistry.get_secret_schemas() + return _get_cached_schemas()🧰 Tools
🪛 Ruff
61-61: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
web/src/components/run-workflow/run-workflow-dialog.tsx (1)
Line range hint
33-47: Enhance error handling and validationWhile the JSON parsing validation is a good addition, consider these improvements:
- Add input validation for
workflowNameto ensure it's not empty.- Include the actual parsing error in the toast message for better debugging.
- Consider trimming the payload before checking for empty string.
Here's a suggested improvement:
const handleRunWorkflow = () => { if (!isActive) { errorToast("You must activate the workflow in order to run it."); return; } + + if (!workflowName?.trim()) { + errorToast("Workflow name is required"); + return; + } let parsedPayload = undefined; try { - if (payloadCache === "") { + const trimmedPayload = payloadCache.trim(); + if (trimmedPayload === "") { parsedPayload = null; } else { - parsedPayload = JSON.parse(payloadCache); + parsedPayload = JSON.parse(trimmedPayload); } } catch (e) { - errorToast("Failed to parse payload into JSON. Is it valid JSON?"); + errorToast(`Invalid JSON payload: ${(e as Error).message}`); return; } triggerWorkflow.mutate({ workflowName, payload: parsedPayload }); };web/src/components/workflow-editor/run-history/workflow-run-trace.tsx (2)
35-37: Consider enhancing the error handling.The current error message is generic and doesn't provide specific details about what went wrong. Consider:
- Passing the actual error message from the API
- Adding retry functionality for transient failures
useEffect(() => { if (data) { setSteps(data); } if (error) { - errorToast("Failed to load workflow runs. Please reload the page."); + errorToast( + `Failed to load workflow runs: ${error.message || 'Unknown error'}. ` + + 'Please try again or reload the page.' + ); } }, [data, error, errorToast]);
Line range hint
43-45: Consider passing error details to ErrorCallout.The ErrorCallout component is rendered without the error information, which could be helpful for debugging.
if (error) { - return <ErrorCallout />; + return <ErrorCallout error={error} />; }web/src/components/api-keys/api-keys.tsx (3)
16-40: Consider UX improvements for the delete operation.While the delete functionality is implemented correctly, consider these UX enhancements:
- Add a loading indicator during deletion (e.g., spinner in the dropdown item)
- Add a confirmation dialog before deletion
- Provide more specific error messages
- Enhance accessibility with ARIA attributes
Here's a suggested implementation:
function DeleteApiKey({ apiKeyId }: DeleteApiKeyProps) { const deleteApiKey = useDeleteApiKey(); const { errorToast } = useToast(); const { removeApiKey } = useApiKeysStore(); const [isDeleting, setIsDeleting] = useState<boolean>(false); + const [showConfirmDialog, setShowConfirmDialog] = useState<boolean>(false); const handleDeleteApiKey = async () => { try { setIsDeleting(true); await deleteApiKey.mutateAsync({ id: apiKeyId }); removeApiKey(apiKeyId); } catch (error) { - errorToast("Failed to delete API key. Please try again."); + errorToast(`Failed to delete API key: ${error.message}`); } finally { setIsDeleting(false); + setShowConfirmDialog(false); } }; return ( - <DropdownMenu.Item disabled={isDeleting} onClick={handleDeleteApiKey}> + <> + <DropdownMenu.Item + disabled={isDeleting} + onClick={() => setShowConfirmDialog(true)} + aria-label="Delete API key" + > <TrashIcon /> - Delete + {isDeleting ? ( + <Flex gap="2" align="center"> + <Spinner size="small" /> + Deleting... + </Flex> + ) : ( + "Delete" + )} </DropdownMenu.Item> + {showConfirmDialog && ( + <AlertDialog> + <AlertDialog.Content> + <AlertDialog.Title>Delete API Key</AlertDialog.Title> + <AlertDialog.Description> + Are you sure you want to delete this API key? This action cannot be undone. + </AlertDialog.Description> + <Flex gap="3" mt="4" justify="end"> + <Button + variant="soft" + onClick={() => setShowConfirmDialog(false)} + > + Cancel + </Button> + <Button + variant="solid" + color="red" + onClick={handleDeleteApiKey} + > + Delete + </Button> + </Flex> + </AlertDialog.Content> + </AlertDialog> + )} + </> ); }
62-101: Enhance table accessibility and user experience.While the table implementation is good, consider these improvements:
- Add loading skeleton for better UX
- Handle empty state
- Use a more consistent date format
- Add accessibility attributes
Here's a suggested implementation:
- <Table.Root> + <Table.Root aria-label="API Keys"> + {apiKeys.length === 0 ? ( + <EmptyState + title="No API Keys" + description="You haven't created any API keys yet." + /> + ) : ( <Table.Header> <Table.Row> <Table.ColumnHeaderCell>Key Name</Table.ColumnHeaderCell> <Table.ColumnHeaderCell>Created</Table.ColumnHeaderCell> <Table.ColumnHeaderCell>Author</Table.ColumnHeaderCell> <Table.ColumnHeaderCell></Table.ColumnHeaderCell> </Table.Row> </Table.Header> <Table.Body> {apiKeys.map((apiKey, idx) => ( <Table.Row key={`api_key_row_${apiKey.name}_${idx}`}> <Table.RowHeaderCell>{apiKey.name}</Table.RowHeaderCell> <Table.Cell> - {apiKey.createdAt.toLocaleString("en-US")} + {new Intl.DateTimeFormat('en-US', { + dateStyle: 'medium', + timeStyle: 'short' + }).format(apiKey.createdAt)} </Table.Cell> <Table.Cell>{apiKey.userEmail}</Table.Cell> <Table.Cell> <Flex justify="end"> <DropdownMenu.Root> <DropdownMenu.Trigger> <IconButton variant="ghost" style={{ cursor: "pointer" }} + aria-label="More options" > <DotsVerticalIcon /> </IconButton> </DropdownMenu.Trigger> <DropdownMenu.Content variant="soft"> <DeleteApiKey apiKeyId={apiKey.id} /> </DropdownMenu.Content> </DropdownMenu.Root> </Flex> </Table.Cell> </Table.Row> ))} </Table.Body> + )} </Table.Root>
Line range hint
52-61: Add loading state UI for better user experience.The current implementation returns null during loading. Consider adding a loading skeleton for better UX.
Here's a suggested implementation:
if (isPending) { - return null; + return ( + <Table.Root> + <Table.Header> + <Table.Row> + <Table.ColumnHeaderCell>Key Name</Table.ColumnHeaderCell> + <Table.ColumnHeaderCell>Created</Table.ColumnHeaderCell> + <Table.ColumnHeaderCell>Author</Table.ColumnHeaderCell> + <Table.ColumnHeaderCell></Table.ColumnHeaderCell> + </Table.Row> + </Table.Header> + <Table.Body> + {[...Array(3)].map((_, idx) => ( + <Table.Row key={idx}> + <Table.Cell><Skeleton width={150} /></Table.Cell> + <Table.Cell><Skeleton width={120} /></Table.Cell> + <Table.Cell><Skeleton width={180} /></Table.Cell> + <Table.Cell><Skeleton width={40} /></Table.Cell> + </Table.Row> + ))} + </Table.Body> + </Table.Root> + ); }web/src/components/workflow-editor/run-history/workflow-run-history.tsx (1)
76-84: LGTM! Consider splitting the effect for better maintainability.The addition of
errorToastto the dependency array is correct as it's used within the effect. However, consider splitting this effect into two separate effects - one for data handling and another for error handling. This would:
- Reduce complexity
- Prevent potential multiple error toasts
- Make the code more maintainable
- useEffect(() => { - if (data && data.length > 0) { - if (runs.length > 0) { - const shiftedSelectedRunIdx = data.findIndex( - (run: TWorkflowRunMetadata) => - run.runId === runs[selectedRunIdx!].runId, - ); - setRuns(data); - setSelectedRunIdx( - shiftedSelectedRunIdx !== -1 ? shiftedSelectedRunIdx : 0, - ); - } else { - setRuns(data); - setSelectedRunIdx(0); - } - } - if (error) { - errorToast("Failed to load workflow runs. Please reload the page."); - } - }, [data, error, setRuns, setSelectedRunIdx, runs, selectedRunIdx, errorToast]); + // Handle data updates + useEffect(() => { + if (data && data.length > 0) { + if (runs.length > 0) { + const shiftedSelectedRunIdx = data.findIndex( + (run: TWorkflowRunMetadata) => + run.runId === runs[selectedRunIdx!].runId, + ); + setRuns(data); + setSelectedRunIdx( + shiftedSelectedRunIdx !== -1 ? shiftedSelectedRunIdx : 0, + ); + } else { + setRuns(data); + setSelectedRunIdx(0); + } + } + }, [data, setRuns, setSelectedRunIdx, runs, selectedRunIdx]); + + // Handle errors + useEffect(() => { + if (error) { + errorToast("Failed to load workflow runs. Please reload the page."); + } + }, [error, errorToast]);admyral/secret/secrets_manager.py (2)
26-36: Enhance docstring with return type and exceptions.The docstring should document:
- Return type:
SecretMetadata- Exceptions:
ValueErrorwhen secret doesn't existasync def update(self, user_id: str, delta_secret: Secret) -> SecretMetadata: """ Update a secret for a user. If the secret does not exist, raise a ValueError. Fields with an empty value are ignored and not updated. Args: user_id: The user id of the user whose secret is updated. delta_secret: The secret to update. + Returns: + SecretMetadata: Metadata of the updated secret. + Raises: + ValueError: If the secret does not exist. """
38-39: Update set method docstring to match implementation.The docstring should be updated to:
- Document the
SecretMetadatareturn type- Clarify the encryption process and its implications
async def set(self, user_id: str, secret: Secret) -> SecretMetadata: """ Set a secret for a user. If the secret already exists, it is overwritten. The function takes care of encrypting the secret. Args: user_id: The user id of the user whose secret is set. secret: The secret to set. + Returns: + SecretMetadata: Metadata of the stored secret, including schema and type. + Note: + The secret is encrypted before storage using the configured encryption mechanism. """web/src/components/workflow-editor/workflow-editor.tsx (1)
99-99: LGTM! Consider refactoring error handling for better maintainability.While adding
errorToastto the dependency array is correct, consider extracting the error handling logic into a separate function for better maintainability.Here's a suggested refactor:
useEffect(() => { + const handleWorkflowError = (error: unknown) => { + if (error instanceof AxiosError && error.response?.status === 404) { + router.push("/"); + errorToast("Workflow does not exist."); + } else { + errorToast("Failed to load workflow. Please refresh the page."); + } + }; + if (workflow) { setWorkflow(prepareForReactFlow(workflow, window.innerWidth)); } if (workflowError) { - if ( - workflowError instanceof AxiosError && - (workflowError as AxiosError).response?.status === 404 - ) { - router.push("/"); - errorToast("Workflow does not exist."); - } else { - errorToast("Failed to load workflow. Please refresh the page."); - } + handleWorkflowError(workflowError); } if (workflow || isNew) { return () => clearWorkflowStore(); } }, [workflow, setWorkflow, isNew, workflowError, clearWorkflowStore, router, errorToast]);web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx (1)
Line range hint
1-215: Consider breaking down this component for better maintainability.The component handles multiple responsibilities and could benefit from being split into smaller, focused sub-components:
- Extract secrets mapping section into
<SecretsMapper />- Extract arguments section into
<ActionArgumentsEditor />- Add type safety for secrets mapping object
- Consider adding error handling for invalid JSON in the code editor
Example structure:
interface SecretsMapperProps { secretsPlaceholders: string[]; secretsMapping: Record<string, string>; onUpdateMapping: (placeholder: string, value: string) => void; } function SecretsMapper({ secretsPlaceholders, secretsMapping, onUpdateMapping }: SecretsMapperProps) { // ... secrets mapping UI } interface ActionArgumentsEditorProps { arguments: TActionMetadata['arguments']; values: string[]; onChange: (argument: string, index: number, value: string) => void; } function ActionArgumentsEditor({ arguments, values, onChange }: ActionArgumentsEditorProps) { // ... arguments editing UI }admyral/actions/integrations/cloud/aws.py (2)
7-12: Well-structured centralization of AWS secret handling!The introduction of
get_aws_secret()helper function andAWSSecretmodel is a good architectural decision that:
- Centralizes AWS credential validation
- Reduces code duplication
- Improves type safety through Pydantic model validation
Consider adding error handling for cases where "AWS_SECRET" is not found in the context.
344-347: LGTM! Consistent secret handling implementation.The change properly implements the new secret management pattern while maintaining the IAM user listing functionality.
Consider adding pagination handling to the IAM users query for large AWS accounts, as the current query might hit API limits.
web/src/stores/workflow-store.ts (2)
Line range hint
262-275: Consider implementing periodic cleanup of lastDeletedEdges.While using a Set for tracking deleted edges is efficient, the
lastDeletedEdgesarray might accumulate over time as it's only cleared when attempting to delete the start node. Consider implementing a cleanup mechanism or limiting the size of this array.onEdgesChange: (changes: EdgeChange[]) => { const deletedEdges = new Set( changes .filter((change) => change.type === "remove") .map((change) => change.id), ); + // Only keep the most recent deleted edges (e.g., last 100) + const MAX_DELETED_EDGES = 100; set({ edges: applyEdgeChanges(changes, get().edges), // Remember the last deleted edges lastDeletedEdges: produce(get().edges, (draft) => - draft.filter((edge) => deletedEdges.has(edge.id)), + draft.filter((edge) => deletedEdges.has(edge.id)).slice(-MAX_DELETED_EDGES), ), }); },
Line range hint
332-335: Address the TODO comment about start node existence.The comment states that a missing start node "cannot happen", but it's better to handle this case properly rather than relying on assumptions. Consider throwing an error or implementing proper error handling.
if (startNodeIdx === -1) { - // TODO: this cannot happen because we always have a start node - return; + throw new Error( + "Start node not found. This indicates a corrupted workflow state." + ); }web/src/components/secrets/secret-row.tsx (4)
145-148: Use consistent date formattingThe
createdAtandupdatedAttimestamps are formatted usingtoLocaleString("en-US"). Relying ontoLocaleStringcan lead to inconsistencies across different browsers and user settings. Consider using a date formatting library likedate-fnsordayjsto ensure consistent and localized date formats throughout the application.
205-208: Avoid inline styles for maintainabilityInline styling is used in the
Dialog.Descriptioncomponent to set the text color. This can make the code harder to maintain and override in the future. Consider using CSS classes, theme tokens, or styled-components to apply styles, which promotes reusability and consistency across the application.
227-228: UseTextcomponent for non-editable textThe "Secret Name" is displayed using a disabled
TextField.Root. For non-editable, display-only text, using aTextcomponent or another static display element improves accessibility and provides a clearer user experience.Apply this diff to update the component:
-<TextField.Root value={secret.secretId} disabled /> +<Text>{secret.secretId}</Text>
280-290: Combine related state into a single arrayCurrently,
dialogState.secretanddialogState.isNeware managed as separate arrays that need to be kept in sync manually. This approach is error-prone and can lead to synchronization bugs. Consider combining these into a single array of objects where each object containskey,value, andisNewproperties. This enhances code readability and reduces the risk of errors.Also applies to: 324-329
web/src/components/secrets/add-secret.tsx (7)
90-93: Use functional update insetDialogStatefor consistencyConsider using the functional form of
setDialogStateto ensure state updates are based on the latest state, preventing potential stale state issues and maintaining consistency throughout the component.Apply this diff:
-setDialogState({ - ...dialogState, - open: false, -}); +setDialogState((draft) => { + draft.open = false; +});
157-167: Reset dialog state upon closing the dialogWhen the dialog is closed, consider resetting
dialogStateto its initial state. This ensures that old data does not persist when the dialog is reopened, providing a clean slate for the user each time.Modify the
onOpenChangehandler as follows:setDialogState((draft) => { - draft.open = false; + Object.assign(draft, { + open: false, + secretId: "", + secretType: null, + secret: [], + error: null, + }); });
35-369: Refactor the component into smaller, reusable componentsThe
AddSecretcomponent is quite large, spanning over 300 lines. Breaking it down into smaller, reusable components can improve readability, maintainability, and make testing easier.Consider extracting parts like the secret fields, custom secret form, and dialog into separate components.
105-361: Extract repeated inline styles into reusable style definitionsMultiple elements use
style={{ cursor: "pointer" }}inline. Extracting these into a CSS class or styled component improves maintainability and ensures consistency across the application.For example:
/* styles.css */ .cursor-pointer { cursor: pointer; }And update the components:
-<Button style={{ cursor: "pointer" }}> +<Button className="cursor-pointer">
225-228: Ensure error messages clear upon input changeWhen correcting the Secret Name after a duplicate name error, the error message may persist until the next validation cycle, which could confuse users.
Consider resetting the
dialogState.errorwhensecretIdchanges:onChange={(event) => setDialogState((draft) => { draft.secretId = event.target.value; + draft.error = null; }) }
231-333: Avoid duplication by unifying secret fields renderingThe rendering logic for secret fields in both standard and custom secrets shares similarities. Consider abstracting this into a separate component or function to reduce code duplication.
This enhances code maintainability and readability.
55-61: Handle loading state and provide user feedbackCurrently, there's no indication to the user that the secret schemas are loading. Consider adding a loading state or spinner to improve user experience.
This can prevent confusion if the data takes time to load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
admyral/actions/integrations/ai/azure_openai.py(2 hunks)admyral/actions/integrations/cloud/aws.py(8 hunks)admyral/secret/secret.py(1 hunks)admyral/secret/secrets_manager.py(3 hunks)admyral/server/endpoints/secret_endpoints.py(2 hunks)web/src/app/(workspace)/settings/api-keys/page.tsx(1 hunks)web/src/app/(workspace)/settings/layout.tsx(1 hunks)web/src/components/api-keys/api-keys.tsx(2 hunks)web/src/components/api-keys/create-api-keys.tsx(2 hunks)web/src/components/navbar/navlink.tsx(1 hunks)web/src/components/publish-workflow-toggle/publish-workflow-toggle-base.tsx(1 hunks)web/src/components/run-workflow/run-workflow-dialog.tsx(1 hunks)web/src/components/secrets/add-secret.tsx(1 hunks)web/src/components/secrets/secret-row.tsx(1 hunks)web/src/components/secrets/secrets.tsx(2 hunks)web/src/components/side-nav-settings/side-nav-item.tsx(1 hunks)web/src/components/side-nav-settings/side-nav-settings.tsx(1 hunks)web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-history.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-step.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-trace.tsx(1 hunks)web/src/components/workflow-editor/workflow-editor.tsx(2 hunks)web/src/hooks/use-set-secret-api.ts(1 hunks)web/src/hooks/use-update-secret-api.ts(1 hunks)web/src/lib/utils.ts(1 hunks)web/src/stores/workflow-store.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- admyral/actions/integrations/ai/azure_openai.py
- admyral/secret/secret.py
- web/src/app/(workspace)/settings/layout.tsx
- web/src/components/api-keys/create-api-keys.tsx
- web/src/components/side-nav-settings/side-nav-settings.tsx
- web/src/hooks/use-update-secret-api.ts
- web/src/lib/utils.ts
🧰 Additional context used
🪛 Ruff
admyral/server/endpoints/secret_endpoints.py
28-28: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
36-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
61-61: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (27)
web/src/app/(workspace)/settings/api-keys/page.tsx (1)
1-4: LGTM! Imports are well-organized.
The imports are logically structured and all are being used within the component.
web/src/hooks/use-set-secret-api.ts (3)
7-7: LGTM: Import changes align with the new type system.
The addition of SecretMetadata and TSecret imports supports the enhanced type safety in the secret management system.
21-26: Verify API response type change compatibility.
The response type has changed from an empty string to SecretMetadata. This is a breaking change that requires verification of all consumers.
#!/bin/bash
# Find all usages of setSecret or useSetSecretApi
echo "Checking for direct API usage..."
rg -A 5 "setSecret\(|useSetSecretApi\(" --type typescript
# Look for response handling patterns
echo "Checking response handling patterns..."
rg -B 5 -A 5 "\.then\(\s*\(.*\)\s*=>" --type typescript15-15: Verify if secretType should be nullable.
Making secretType nullable might lead to inconsistencies in secret type tracking. Consider making it required if all secrets should have a type.
web/src/components/secrets/secrets.tsx (3)
1-9: LGTM! Imports are well-organized.
The imports are clean and properly structured, with the client directive correctly placed at the top.
34-55: Verify SecretRow implementation for secure display of sensitive data.
The table displays sensitive information. Let's verify that the SecretRow component:
- Properly masks sensitive values
- Implements secure copy mechanisms
- Handles permissions correctly
#!/bin/bash
# Description: Check SecretRow implementation for security measures
# Test: Look for proper masking and secure handling of sensitive data
ast-grep --pattern 'export default function SecretRow($_) {
$$$
}'
# Check for any direct secret value rendering
rg -A 5 "secret\." --type tsx11-17: Verify secure handling of secrets in the store.
While the store implementation looks clean, ensure that the secrets store (useSecretsStore) implements proper security measures:
- Memory cleanup after usage
- No persistent storage of unencrypted secrets
- Protection against memory leaks
admyral/server/endpoints/secret_endpoints.py (1)
14-20: Verify the string encoding transformation is necessary and safe
The handle_secret_values function performs a UTF-8 encode/decode cycle with unicode escape sequences. This could lead to unexpected results if:
- The input strings are already escaped
- The secrets contain valid escape sequences that should be preserved
web/src/components/run-workflow/run-workflow-dialog.tsx (1)
30-30: LGTM: Proper dependency list management
The addition of infoToast and errorToast to the dependency array is correct and prevents potential stale closure issues.
web/src/components/workflow-editor/run-history/workflow-run-trace.tsx (1)
36-36: LGTM! Good catch on adding errorToast to the dependency array.
The addition of errorToast to the useEffect dependencies is correct and necessary to avoid potential stale closure issues with the error notification callback.
web/src/components/api-keys/api-keys.tsx (2)
3-14: LGTM! Clean imports and well-defined interface.
The new imports and interface definition are properly organized and provide good type safety for the delete functionality.
44-44: LGTM! Good store usage.
The store integration is clean and properly destructured with all necessary methods.
web/src/components/workflow-editor/run-history/workflow-run-history.tsx (1)
76-84: Address the TODO comment about potential data loss.
The TODO comment indicates a potential issue where selected runs could be lost due to the 100-run limit. Consider implementing one of these solutions:
- Add a warning when the selected run is about to be lost
- Implement pagination to access older runs
- Store the selected run ID separately and fetch its data if needed
Let's check if this limitation is documented:
Would you like me to help implement one of these solutions or create an issue to track this enhancement?
web/src/components/workflow-editor/run-history/workflow-run-step.tsx (1)
31-31: LGTM! Correctly adds errorToast to useEffect dependencies.
The change properly includes the errorToast function in the useEffect dependency array, following React's hooks exhaustive-deps rule and preventing potential stale closure issues.
Let's verify if similar patterns exist in other components that might need the same fix:
✅ Verification successful
The previous pattern might have been too specific. Let's try a broader search to find any useEffect hooks that use errorToast, regardless of the exact error handling pattern.
Let's search for useEffect hooks with error or errorToast in their dependency arrays to find similar patterns.
Based on the file content and our verification attempts, I can now generate the final response:
✅ Correctly adds errorToast to useEffect dependencies
The change properly includes the errorToast function in the useEffect dependency array. Looking at the component's implementation:
- The useEffect hook correctly handles error states using errorToast
- The dependency array now includes both error and errorToast, preventing stale closure issues
- The implementation follows React's hooks exhaustive-deps rule
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find similar useEffect patterns with error handling that might be missing errorToast in dependencies
# Search for useEffect hooks that use errorToast but might be missing it in dependencies
ast-grep --pattern 'useEffect(() => {
if ($error) {
$$$errorToast$$$
}
}, [$$$])'
Length of output: 91
Script:
#!/bin/bash
# Let's try a broader search pattern to find useEffect hooks with error handling
ast-grep --pattern 'useEffect(() => {
$$$
errorToast($$$)
$$$
})'
# Also search for direct text patterns using ripgrep
rg "useEffect.*errorToast.*\[.*\]" -t typescript
Length of output: 159
Script:
#!/bin/bash
# Let's try a broader search pattern to find useEffect hooks with error handling
ast-grep --pattern 'useEffect(() => {
$$$
errorToast($$$)
$$$
})'
# Search for files containing both useEffect and errorToast
rg "useEffect.*errorToast|errorToast.*useEffect" --type=ts --type=tsx -l
# Also get the content of the current file to understand the context better
cat web/src/components/workflow-editor/run-history/workflow-run-step.tsx
Length of output: 4632
Script:
#!/bin/bash
# Let's try one more time with ripgrep without type specification
rg "useEffect.*\[.*error.*\]" --type-add 'ts:*.{ts,tsx}' --type=ts
# Also search for files that might use errorToast in useEffect
rg "useEffect.*\[.*errorToast.*\]" --type-add 'ts:*.{ts,tsx}' --type=ts
Length of output: 142
admyral/secret/secrets_manager.py (2)
9-10: LGTM! Required imports for new functionality.
The new imports support the delta update functionality and secret type validation.
99-106: Review security implications of schema modification.
The ability to modify schema for unregistered secret types could lead to potential security issues:
- Data leakage through selective field removal
- No validation of field names for custom secrets
Consider:
- Adding validation for custom secret field names
- Logging schema modifications for audit purposes
- Adding rate limiting for schema changes
✅ Verification successful
Let me gather more information about the security controls and validation around secret modifications.
Let me check for validation and security controls around secret modifications.
Let me check one more aspect about secret validation and schema enforcement.
Based on the gathered information, I can now provide a final response about the security implications of schema modification.
Schema modification controls are adequately implemented
The current implementation has several security controls in place:
- Schema modification is only allowed for unregistered (custom) secrets, enforced by
SecretRegistry.is_registered() - Registered secrets use Pydantic models for schema validation, ensuring type safety and field validation
- Secret values are encrypted before storage (
encrypt_secret) - Access control is enforced through user authentication and ownership checks
The ability to modify schema for custom secrets is an intentional feature, not a security issue, as it:
- Only affects the specific user's own secrets
- Maintains encryption of secret values
- Preserves schema validation for registered secret types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential security controls around secret modifications
rg -A 5 "store_secret|update_secret"
Length of output: 1638
Script:
#!/bin/bash
# Check for secret validation and handling
rg -A 10 "handle_secret_values|validate_secret"
# Check for audit logging
rg -A 5 "log\.|logger\."
# Check SecretRegistry implementation
ast-grep --pattern 'class SecretRegistry'
Length of output: 50807
Script:
#!/bin/bash
# Check for secret validation and schema handling
rg -A 10 "SecretRegistry|secret_type|secret\.secret" admyral/secret/
# Check for secret schema validation
ast-grep --pattern 'def validate_secret'
Length of output: 8152
Script:
#!/bin/bash
# Check for secret model and schema validation
rg -A 10 "class Secret|BaseModel.*Secret" admyral/secret/
rg -A 5 "pydantic" admyral/secret/
Length of output: 3779
web/src/components/workflow-editor/workflow-editor.tsx (1)
57-57: LGTM! Correctly added errorToast to dependencies.
The addition of errorToast to the dependency array follows React's hooks best practices and ensures consistent error handling behavior.
web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx (1)
39-39: LGTM! Correctly added updateArgs to useEffect dependencies.
The addition of updateArgs to the dependency array is correct and follows React's hooks exhaustive dependencies rule. Since updateArgs is stable (provided by useImmer), this won't cause unnecessary re-renders or infinite loops.
admyral/actions/integrations/cloud/aws.py (6)
30-33: LGTM! Clean transition to the new secret handling.
The change correctly implements the new secret retrieval pattern while maintaining the existing functionality.
63-66: LGTM! Consistent implementation of new secret handling.
The change maintains the existing query logic while adopting the new secret management approach.
116-119: LGTM! Proper adoption of the new secret pattern.
The change correctly implements the new secret handling while preserving the SSL enforcement logic.
190-193: LGTM! Consistent implementation of secret handling.
The change properly adopts the new secret management pattern while maintaining the public access check logic.
268-271: LGTM! Clean adoption of new secret pattern.
The change correctly implements the new secret handling while preserving the write access control logic.
301-304: LGTM! Proper implementation of new secret handling.
The change correctly adopts the new secret management pattern while maintaining the encryption check logic.
web/src/stores/workflow-store.ts (2)
235-237: LGTM! Performance improvement in start node removal check.
The change from filter().length > 0 to some() is a good optimization. The some() method stops iteration as soon as it finds a match, making it more efficient than filter which processes all elements.
250-250: LGTM! Consistent optimization pattern.
The change to use some() maintains consistency with the previous optimization and correctly handles the deletion check.
web/src/components/secrets/add-secret.tsx (1)
95-96: Log error details for better debugging
In the catch block of handleSaveSecret, consider logging the actual error to aid in debugging while keeping user-facing messages generic.
Apply this diff:
} catch (error) {
errorToast("Failed to save secret. Please try again.");
+ console.error("Error saving secret:", error);
} finally {It's important to ensure that this does not expose sensitive information in production logs.
| import { DISABLE_AUTH } from "@/constants/env"; | ||
| import { Flex, Text } from "@radix-ui/themes"; | ||
|
|
||
| export default async function ApiKeysPage() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary async declaration.
The component is marked as async but doesn't contain any await operations.
-export default async function ApiKeysPage() {
+export default function ApiKeysPage() {📝 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.
| export default async function ApiKeysPage() { | |
| export default function ApiKeysPage() { |
| @router.post("/update", status_code=status.HTTP_201_CREATED) | ||
| async def update_secret( | ||
| secret: Secret, authenticated_user: AuthenticatedUser = Depends(authenticate) | ||
| ) -> SecretMetadata: | ||
| secret.secret = handle_secret_values(secret.secret) | ||
| return await get_secrets_manager().update(authenticated_user.user_id, secret) |
There was a problem hiding this comment.
Consider adjusting status code and adding validation
- For updates, HTTP 200 OK is more appropriate than 201 CREATED
- Consider adding validation to ensure the secret exists before update
- Consider adding optimistic locking to prevent concurrent modifications
Apply this diff:
-@router.post("/update", status_code=status.HTTP_201_CREATED)
+@router.post("/update", status_code=status.HTTP_200_OK)
async def update_secret(
secret: Secret, authenticated_user: AuthenticatedUser = Depends(authenticate)
) -> SecretMetadata:
+ # Verify secret exists
+ existing = await get_secrets_manager().get(authenticated_user.user_id, secret.id)
+ if not existing:
+ raise HTTPException(status_code=404, detail="Secret not found")
secret.secret = handle_secret_values(secret.secret)
return await get_secrets_manager().update(authenticated_user.user_id, secret)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff
36-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
admyral/secret/secrets_manager.py
Outdated
| async def set(self, user_id: str, secret: Secret) -> SecretMetadata: | ||
| serialized_secret = json.dumps(secret.secret) | ||
| encrypted_secret = encrypt_secret(serialized_secret) | ||
| secret_schema = list(secret.secret.keys()) | ||
| return await self.db.store_secret( | ||
| user_id, secret.secret_id, encrypted_secret, secret_schema | ||
| user_id, | ||
| secret.secret_id, | ||
| encrypted_secret, | ||
| secret_schema, | ||
| secret.secret_type, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and size limits for secret storage.
Consider adding:
- Schema validation against registered secret types
- Size limits for serialized secrets to prevent DoS
- JSON schema validation for custom secret types
async def set(self, user_id: str, secret: Secret) -> SecretMetadata:
+ if SecretRegistry.is_registered(secret.secret_type):
+ SecretRegistry.validate_schema(secret)
+
serialized_secret = json.dumps(secret.secret)
+ if len(serialized_secret) > MAX_SECRET_SIZE:
+ raise ValueError(f"Secret size exceeds maximum allowed size of {MAX_SECRET_SIZE} bytes")
+
encrypted_secret = encrypt_secret(serialized_secret)Committable suggestion skipped: line range outside the PR's diff.
admyral/secret/secrets_manager.py
Outdated
| async def update(self, user_id: str, delta_secret: Secret) -> SecretMetadata: | ||
| secret = await self.get(user_id, delta_secret.secret_id) | ||
| if not secret: | ||
| raise ValueError(f"Secret {delta_secret.secret_id} does not exist") | ||
|
|
There was a problem hiding this comment.
Add transaction to prevent race conditions.
The get-then-set pattern could lead to race conditions if multiple updates occur simultaneously. Consider using a database transaction.
async def update(self, user_id: str, delta_secret: Secret) -> SecretMetadata:
+ async with self.db.transaction():
secret = await self.get(user_id, delta_secret.secret_id)
if not secret:
raise ValueError(f"Secret {delta_secret.secret_id} does not exist")Committable suggestion skipped: line range outside the PR's diff.
admyral/secret/secrets_manager.py
Outdated
| for key, value in delta_secret.secret.items(): | ||
| # ignore empty values - empty values in the delta mean that no | ||
| # update was provided for the key-value pair. | ||
| if is_empty(value): | ||
| continue | ||
| secret.secret[key] = value | ||
|
|
There was a problem hiding this comment.
Validate secret type consistency.
The update method should verify that the delta_secret's type matches the existing secret's type to prevent type confusion attacks.
async def update(self, user_id: str, delta_secret: Secret) -> SecretMetadata:
secret = await self.get(user_id, delta_secret.secret_id)
if not secret:
raise ValueError(f"Secret {delta_secret.secret_id} does not exist")
+ if delta_secret.secret_type != secret.secret_type:
+ raise ValueError(f"Secret type mismatch: expected {secret.secret_type}, got {delta_secret.secret_type}")
+
for key, value in delta_secret.secret.items():📝 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.
| for key, value in delta_secret.secret.items(): | |
| # ignore empty values - empty values in the delta mean that no | |
| # update was provided for the key-value pair. | |
| if is_empty(value): | |
| continue | |
| secret.secret[key] = value | |
| for key, value in delta_secret.secret.items(): | |
| # ignore empty values - empty values in the delta mean that no | |
| # update was provided for the key-value pair. | |
| if is_empty(value): | |
| continue | |
| secret.secret[key] = value | |
| function hasNonEmptyValueFieldsForNewFields( | ||
| secret: { key: string; value: string }[], | ||
| isNew: boolean[], | ||
| ): boolean { | ||
| return isNew.some( | ||
| (isFieldNew, idx) => isFieldNew && secret[idx].value.length === 0, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename function for clarity
The function hasNonEmptyValueFieldsForNewFields returns true when any newly added field has an empty value, which indicates the presence of empty values in new fields. The function name suggests that it checks for non-empty values, which can be misleading. To improve readability and avoid confusion, consider renaming the function to hasEmptyValueFieldsForNewFields or hasNewFieldsWithEmptyValues to better reflect its purpose.
| href="https://docs.admyral.dev/integrations/integrations" | ||
| target="_blank" | ||
| > |
There was a problem hiding this comment.
Add rel attribute to external links for security
The anchor tag linking to external documentation uses target="_blank" without a rel attribute. This can expose the application to security risks such as reverse tabnabbing. Add rel="noopener noreferrer" to the anchor tag to mitigate these risks.
Apply this diff to fix the issue:
213 href="https://docs.admyral.dev/integrations/integrations"
214 target="_blank"
+ rel="noopener noreferrer"📝 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.
| href="https://docs.admyral.dev/integrations/integrations" | |
| target="_blank" | |
| > | |
| href="https://docs.admyral.dev/integrations/integrations" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > |
| <Dialog.Root | ||
| open={dialogState.open} | ||
| onOpenChange={(_) => { | ||
| if (isSaving) { | ||
| // we don't allow to close while the secret is saving | ||
| return; | ||
| } | ||
| setDialogState((draft) => { | ||
| draft.open = false; | ||
| }); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent dialog from becoming non-responsive
While preventing the dialog from closing during saving is good, ensure that there is a timeout or error handling in case the saving process hangs, to prevent the dialog from becoming permanently non-responsive.
Consider implementing a timeout or allowing the user to cancel the saving process after a certain period.
| const handleSaveSecret = async () => { | ||
| try { | ||
| setIsSaving(true); | ||
| setDialogState((draft) => { | ||
| draft.error = null; | ||
| }); | ||
|
|
||
| if (hasDuplicateSecretKeyFields(dialogState.secret)) { | ||
| setDialogState((draft) => { | ||
| draft.error = "Key fields must be unique."; | ||
| }); | ||
| return; | ||
| } | ||
| if (hasEmptyKeyOrValue(dialogState.secret)) { | ||
| setDialogState((draft) => { | ||
| draft.error = "Key and value fields must not be empty."; | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const newSecret = await setSecret.mutateAsync({ | ||
| secretId: dialogState.secretId, | ||
| secretType: dialogState.secretType, | ||
| secret: dialogState.secret, | ||
| }); | ||
| addNewSecret(newSecret); | ||
|
|
||
| setDialogState({ | ||
| ...dialogState, | ||
| open: false, | ||
| }); | ||
| } catch (error) { | ||
| errorToast("Failed to save secret. Please try again."); | ||
| } finally { | ||
| setIsSaving(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add validation for empty Secret Name in handleSaveSecret
Currently, handleSaveSecret does not validate if the Secret Name (secretId) is empty. Adding this validation ensures that even if the button's disabled state is bypassed, the application remains robust against empty input.
Apply this diff to include the validation:
+if (dialogState.secretId.trim().length === 0) {
+ setDialogState((draft) => {
+ draft.error = "Secret Name must not be empty.";
+ });
+ return;
+}📝 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.
| const handleSaveSecret = async () => { | |
| try { | |
| setIsSaving(true); | |
| setDialogState((draft) => { | |
| draft.error = null; | |
| }); | |
| if (hasDuplicateSecretKeyFields(dialogState.secret)) { | |
| setDialogState((draft) => { | |
| draft.error = "Key fields must be unique."; | |
| }); | |
| return; | |
| } | |
| if (hasEmptyKeyOrValue(dialogState.secret)) { | |
| setDialogState((draft) => { | |
| draft.error = "Key and value fields must not be empty."; | |
| }); | |
| return; | |
| } | |
| const newSecret = await setSecret.mutateAsync({ | |
| secretId: dialogState.secretId, | |
| secretType: dialogState.secretType, | |
| secret: dialogState.secret, | |
| }); | |
| addNewSecret(newSecret); | |
| setDialogState({ | |
| ...dialogState, | |
| open: false, | |
| }); | |
| } catch (error) { | |
| errorToast("Failed to save secret. Please try again."); | |
| } finally { | |
| setIsSaving(false); | |
| } | |
| }; | |
| const handleSaveSecret = async () => { | |
| try { | |
| setIsSaving(true); | |
| setDialogState((draft) => { | |
| draft.error = null; | |
| }); | |
| if (dialogState.secretId.trim().length === 0) { | |
| setDialogState((draft) => { | |
| draft.error = "Secret Name must not be empty."; | |
| }); | |
| return; | |
| } | |
| if (hasDuplicateSecretKeyFields(dialogState.secret)) { | |
| setDialogState((draft) => { | |
| draft.error = "Key fields must be unique."; | |
| }); | |
| return; | |
| } | |
| if (hasEmptyKeyOrValue(dialogState.secret)) { | |
| setDialogState((draft) => { | |
| draft.error = "Key and value fields must not be empty."; | |
| }); | |
| return; | |
| } | |
| const newSecret = await setSecret.mutateAsync({ | |
| secretId: dialogState.secretId, | |
| secretType: dialogState.secretType, | |
| secret: dialogState.secret, | |
| }); | |
| addNewSecret(newSecret); | |
| setDialogState({ | |
| ...dialogState, | |
| open: false, | |
| }); | |
| } catch (error) { | |
| errorToast("Failed to save secret. Please try again."); | |
| } finally { | |
| setIsSaving(false); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
admyral/db/admyral_store.py (2)
162-187: Optimize API key creation by using RETURNING clause.The method performs an additional query to fetch user email after inserting the API key. This can be optimized using a single query with RETURNING clause.
Consider this optimization:
- await db.exec( - insert(ApiKeySchema).values( - id=api_key_id, - user_id=user_id, - name=name, - key=key, - ) - ) - await db.commit() - - result = await db.exec( - select(ApiKeySchema, UserSchema) - .join(UserSchema) - .where(ApiKeySchema.user_id == user_id) - .where(ApiKeySchema.id == api_key_id) - .where(UserSchema.id == user_id) - ) - row = result.one() + result = await db.exec( + insert(ApiKeySchema) + .values( + id=api_key_id, + user_id=user_id, + name=name, + key=key, + ) + .returning(ApiKeySchema, UserSchema) + .select_from(UserSchema) + .where(UserSchema.id == user_id) + ) + row = result.one() + await db.commit()
Line range hint
929-975: Optimize secret storage with fewer queries.The method performs separate queries for secret operations and user lookup. This can be optimized using a single query.
Consider this optimization:
- secret_result = await db.exec( - update(SecretsSchema) - .where(SecretsSchema.user_id == user_id) - .where(SecretsSchema.secret_id == secret_id) - .values( - encrypted_secret=encrypted_secret, - schema_json_serialized=json.dumps(schema), - updated_at=utc_now(), - ) - .returning(SecretsSchema.created_at, SecretsSchema.updated_at) - ) - secret_created_at_updated_at = secret_result.one() - - await db.commit() - - result = await db.exec(select(UserSchema).where(UserSchema.id == user_id)) - user = result.one() + result = await db.exec( + update(SecretsSchema) + .where(SecretsSchema.user_id == user_id) + .where(SecretsSchema.secret_id == secret_id) + .values( + encrypted_secret=encrypted_secret, + schema_json_serialized=json.dumps(schema), + updated_at=utc_now(), + ) + .returning(SecretsSchema, UserSchema) + .select_from(UserSchema) + .where(UserSchema.id == user_id) + ) + row = result.one() + await db.commit()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
admyral/db/admyral_store.py(18 hunks)admyral/db/store_interface.py(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- admyral/db/store_interface.py
🔇 Additional comments (3)
admyral/db/admyral_store.py (3)
192-207: LGTM! Type-safe model validation.
The use of model_validate for creating ApiKey objects ensures type safety and proper validation.
210-210: LGTM! Modern type hint syntax.
The updates to use | instead of Optional align with modern Python type hint syntax (PEP 604) and improve code readability.
Also applies to: 246-246, 323-323, 405-405, 422-422, 485-485, 526-526, 555-555, 707-707, 761-761, 920-920, 929-929
889-907: LGTM! Comprehensive secret metadata.
The method now includes user email in the secret metadata, improving the data available for display and auditing purposes.
admyral/db/admyral_store.py
Outdated
| async def delete_user(self, user_id: str) -> None: | ||
| async with self._get_async_session() as db: | ||
| await db.exec(delete(UserSchema).where(UserSchema.id == user_id)) | ||
| await db.commit() |
There was a problem hiding this comment.
Consider implementing cascading deletes for user-related data.
The delete_user method should handle the deletion of associated data (API keys, secrets, workflows, etc.) to maintain data integrity and prevent orphaned records.
Consider implementing a transaction that deletes related data before deleting the user:
async def delete_user(self, user_id: str) -> None:
async with self._get_async_session() as db:
+ # Delete associated data first
+ await db.exec(delete(ApiKeySchema).where(ApiKeySchema.user_id == user_id))
+ await db.exec(delete(SecretsSchema).where(SecretsSchema.user_id == user_id))
+ await db.exec(delete(WorkflowSchema).where(WorkflowSchema.user_id == user_id))
await db.exec(delete(UserSchema).where(UserSchema.id == user_id))
await db.commit()Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (23)
admyral/models/secret.py (2)
9-9: Consider adding validation for secret_type valuesSince this field will be used to categorize different types of secrets (e.g., Anthropic, Azure OpenAI, MistralAI), consider adding validation to ensure only valid secret types are accepted.
- secret_type: str | None + secret_type: str | None = Field( + None, + description="Type of the secret (e.g., 'anthropic', 'azure_openai', 'mistral_ai')" + )
Line range hint
1-28: Consider updating documentation to reflect these changesThe changes introduce significant enhancements to secret management, including:
- Secret type categorization
- User association via email
- Audit timestamps
- Enhanced metadata tracking
These changes seem more extensive than just "settings rework" as mentioned in the PR title.
Would you like assistance in updating the documentation to reflect these changes?
admyral/db/schemas/secrets_schemas.py (2)
38-39: Consider enhancing the secret_type field documentation and validation.While the field is properly defined, consider these improvements:
- Enhance the documentation to specify:
- Expected values/formats
- Use cases for different secret types
- Whether this maps to specific integrations
- Consider using an Enum for type safety and validation
Example implementation:
from enum import Enum class SecretType(str, Enum): API_KEY = "api_key" OAUTH_TOKEN = "oauth_token" PASSWORD = "password" # Add other relevant types secret_type: Optional[SecretType] = Field(sa_type=TEXT(), nullable=True) """ Type of the secret. Valid values: - api_key: For API key credentials - oauth_token: For OAuth access tokens - password: For password credentials """
Line range hint
1-52: Consider implementing schema validation based on secret_type.Since different types of secrets might have different schema requirements, consider implementing type-specific schema validation. This could help ensure that secrets of different types maintain their expected structure.
Potential approaches:
- Define type-specific schema validators in a separate module
- Use Pydantic's discriminated unions to handle different secret types
- Implement a factory pattern for creating and validating secrets based on their type
Would you like me to elaborate on any of these approaches?
web/src/components/secrets/secrets.tsx (1)
34-67: Consider these improvements to the table implementation.While the table structure is good, there are several potential improvements:
- Move inline styles to a separate stylesheet for better maintainability
- Consider using responsive widths instead of fixed pixel/percentage values
- Add aria-labels to empty column header cells for better accessibility
- Simplify the key prop
Here's a suggested refactor:
<Table.Root> <Table.Header> <Table.Row> <Table.ColumnHeaderCell - style={{ width: "24px" }} + className="actions-column" + aria-label="Actions column" ></Table.ColumnHeaderCell> <Table.ColumnHeaderCell - style={{ width: "30%" }} + className="name-column" > Name </Table.ColumnHeaderCell> {/* Similar changes for other columns */} </Table.Row> </Table.Header> <Table.Body> {secrets.map((secret, idx) => ( <SecretRow - key={`secret_row_${secret.secretId}`} + key={secret.secretId} secret={secret} idx={idx} /> ))} </Table.Body> </Table.Root>Add to your stylesheet:
.actions-column { width: var(--actions-column-width, 24px); } .name-column { width: var(--name-column-width, 30%); } /* Add similar styles for other columns */admyral/server/endpoints/secret_endpoints.py (2)
26-31: LGTM! Status code and return type changes are appropriate.The HTTP 201 status code correctly indicates resource creation, and returning
SecretMetadataprovides valuable information to clients.Consider addressing the Depends usage warning by moving it into the function:
@router.post("/set", status_code=status.HTTP_201_CREATED) -async def set_secret( - secret: Secret, authenticated_user: AuthenticatedUser = Depends(authenticate) -) -> SecretMetadata: +async def set_secret(secret: Secret) -> SecretMetadata: + authenticated_user = Depends(authenticate) secret.secret = handle_secret_values(secret.secret) return await get_secrets_manager().set(authenticated_user.user_id, secret)🧰 Tools
🪛 Ruff
28-28: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
59-63: Consider adding response caching.Since secret schemas are likely static and identical for all users, consider adding response caching to improve performance and reduce unnecessary computations.
+from fastapi import Response + @router.get("/schemas") async def get_secret_schemas( _authenticated_user: AuthenticatedUser = Depends(authenticate), + response: Response ) -> dict[str, list[str]]: + response.headers["Cache-Control"] = "public, max-age=3600" # Cache for 1 hour return SecretRegistry.get_secret_schemas()🧰 Tools
🪛 Ruff
61-61: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
web/src/components/navbar/navbar.tsx (1)
Line range hint
88-92: Fix incorrect alt text for GitHub logo.The alt text for the GitHub logo image is incorrectly set to "Slack". This should be updated to maintain accessibility standards.
<Image src="/github_logo.svg" - alt="Slack" + alt="GitHub" width={18} height={18} />admyral/secret/secrets_manager.py (1)
25-35: Enhance docstring for update method.The docstring should document additional important behaviors:
- Schema validation for registered secret types
- Schema modification behavior for unregistered (custom) secret types
- Type consistency requirements
Add these details to the docstring:
async def update(self, user_id: str, delta_secret: Secret) -> SecretMetadata: """ Update a secret for a user. If the secret does not exist, raise a ValueError. Fields with an empty value are ignored and not updated. + + For registered secret types: + - Schema is validated against the registered type + - Keys can only be updated, not removed + + For unregistered (custom) secret types: + - Schema can be modified by removing keys not present in delta_secret + - No schema validation is performed + + The secret type cannot be changed during updates. Args: user_id: The user id of the user whose secret is updated. delta_secret: The secret to update. """web/src/components/secrets/add-secret.tsx (3)
35-53: Add type definitions and loading state handling.Consider the following improvements:
- Add TypeScript interfaces for the API responses from
useGetSecretsSchemas.- Add a loading state for the initial schema fetch to prevent potential UI flickers.
+interface SecretSchema { + [key: string]: string[]; +} -const { data: secretsSchemas, error } = useGetSecretsSchemas(); +const { data: secretsSchemas, error, isLoading } = useGetSecretsSchemas();
63-109: Add cleanup for ongoing save operations.Consider adding a cleanup mechanism to handle component unmounting during an active save operation to prevent state updates on an unmounted component.
+const [abortController, setAbortController] = useState<AbortController | null>(null); const handleSaveSecret = async () => { + const controller = new AbortController(); + setAbortController(controller); try { setIsSaving(true); // ... existing code ... const newSecret = await setSecret.mutateAsync({ secretId: dialogState.secretId.trim(), secretType: dialogState.secretType, secret: dialogState.secret, + signal: controller.signal, }); // ... existing code ... } finally { setIsSaving(false); + setAbortController(null); } }; +useEffect(() => { + return () => { + if (abortController) { + abortController.abort(); + } + }; +}, [abortController]);
111-380: Enhance accessibility and error handling.
- Add aria-labels to improve accessibility:
-<Button style={{ cursor: "pointer" }}> +<Button style={{ cursor: "pointer" }} aria-label="Add new secret"> -<IconButton variant="soft" color="red" size="1" +<IconButton + variant="soft" + color="red" + size="1" + aria-label="Remove field"
- Consider wrapping the component with an error boundary to gracefully handle rendering errors:
import { ErrorBoundary } from 'react-error-boundary'; function ErrorFallback({error}: {error: Error}) { return ( <Flex direction="column" gap="2"> <Text color="red">Something went wrong:</Text> <Text size="2">{error.message}</Text> </Flex> ); } export default function AddSecretWithErrorBoundary() { return ( <ErrorBoundary FallbackComponent={ErrorFallback}> <AddSecret /> </ErrorBoundary> ); }web/src/components/secrets/secret-row.tsx (5)
50-60: Consider splitting dialog state for better maintainabilityThe current dialog state combines multiple concerns into a single object. Consider splitting it into separate states for better maintainability and readability:
-const [dialogState, setDialogState] = useImmer<{ - open: boolean; - secret: { key: string; value: string }[]; - isNew: boolean[]; - error: string | null; -}>({ - open: false, - secret: secret.secretSchema.map((key) => ({ key, value: "" })), - isNew: Array(secret.secretSchema.length).fill(false), - error: null, -}); +const [isOpen, setIsOpen] = useState(false); +const [secretFields, setSecretFields] = useImmer( + secret.secretSchema.map((key) => ({ key, value: "" })) +); +const [newFieldFlags, setNewFieldFlags] = useState( + Array(secret.secretSchema.length).fill(false) +); +const [error, setError] = useState<string | null>(null);
64-77: Add loading state for delete operationThe delete operation should show a loading state to prevent multiple clicks and improve user feedback.
+const [isDeleting, setIsDeleting] = useState(false); const handleDeleteSecret = async () => { try { + setIsDeleting(true); setDialogState((draft) => ({ open: false, secret: secret.secretSchema.map((key) => ({ key, value: "" })), isNew: Array(secret.secretSchema.length).fill(false), error: null, })); await deleteSecret.mutateAsync({ secretId: secrets[idx].secretId }); removeSecret(idx); } catch (error) { errorToast("Failed to delete secret. Please try again."); + } finally { + setIsDeleting(false); } };
79-123: Improve error handling in update operationThe error handling in
handleUpdateSecretcould be more specific and provide better feedback to users.} catch (error) { + const errorMessage = error instanceof Error + ? error.message + : "Failed to update secret. Please try again or contact support if the problem persists."; setDialogState((draft) => { - draft.error = "Failed to update secret. Please try again or contact support if the problem persists."; + draft.error = errorMessage; }); }
127-141: Optimize click handler and improve accessibilityThe click handler recreates the state object on every click. Consider extracting it to a memoized function. Also, add proper ARIA labels for better accessibility.
+const handleRowClick = useCallback(() => { + setDialogState((draft) => { + draft.open = true; + draft.secret = secret.secretSchema.map((key) => ({ + key, + value: "", + })); + draft.isNew = Array(secret.secretSchema.length).fill(false); + }); +}, [secret.secretSchema, setDialogState]); <Table.Row className="hover:bg-gray-50 cursor-pointer" + role="button" + aria-label={`Edit secret ${secret.secretId}`} - onClick={() => - setDialogState((draft) => { - draft.open = true; - draft.secret = secret.secretSchema.map((key) => ({ - key, - value: "", - })); - draft.isNew = Array(secret.secretSchema.length).fill(false); - }) - } + onClick={handleRowClick} >
165-172: Add ARIA label to dropdown triggerThe dropdown trigger button lacks an accessibility label.
<DropdownMenu.Trigger> <IconButton variant="ghost" style={{ cursor: "pointer" }} + aria-label="More options" > <DotsVerticalIcon /> </IconButton> </DropdownMenu.Trigger>admyral/db/admyral_store.py (3)
174-188: Consider adding an index for the user_id join condition.The join between
ApiKeySchemaandUserSchemaonuser_idcould benefit from an index to improve query performance, especially as the number of API keys grows.
1005-1011: Improve error messages for better debugging.The error messages could be more specific about what went wrong:
- f"Failed to update secret because the secret {secret_id} was not found." + f"Failed to update secret: secret '{secret_id}' not found for user '{user_id}'." - "Failed to update secret because the secret was modified simultaneously." + f"Concurrent modification detected: secret '{secret_id}' was modified by another operation."
932-935: Ensure consistent parameter ordering between related methods.The parameter ordering differs between
store_secretandcompare_and_swap_secret. Consider keeping them consistent for better maintainability.async def store_secret( self, user_id: str, secret_id: str, - encrypted_secret: str | None, - schema: list[str], - secret_type: str | None = None, + encrypted_secret: str | None, + secret_schema: list[str], # Rename for consistency + secret_type: str | None = None, ) -> SecretMetadata:Also applies to: 998-1000
web/src/lib/api.ts (3)
5-5: Unify 'zod' imports for consistencyCurrently, you're importing both
zandzodfrom the"zod"library, which can lead to confusion and inconsistency. It's recommended to importzas the main namespace and use it consistently throughout the code.Apply this diff to unify the imports:
-import zod from "zod"; +import { z } from "zod";Also, update instances of
zod.ZodErrortoz.ZodErroraccordingly.
86-87: Update error checking to match the unified importAfter unifying the imports, ensure that the error instance check uses the correct namespace.
Apply this diff:
-if (error instanceof zod.ZodError) { +if (error instanceof z.ZodError) {
98-101: Preserve original error for better debuggingThrowing a new generic
Errormay lose the original stack trace and make debugging harder. Consider rethrowing the original error to preserve the complete stack trace and error context.Apply this diff:
-throw new Error( - `Unexpected error: ${error instanceof Error ? error.message : String(error)}` -); +throw error;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
admyral/db/admyral_store.py(18 hunks)admyral/db/schemas/secrets_schemas.py(3 hunks)admyral/db/store_interface.py(10 hunks)admyral/models/secret.py(1 hunks)admyral/secret/secret_registry.py(1 hunks)admyral/secret/secrets_manager.py(4 hunks)admyral/server/endpoints/secret_endpoints.py(2 hunks)web/src/app/(workspace)/settings/api-keys/page.tsx(1 hunks)web/src/app/(workspace)/settings/secrets/page.tsx(1 hunks)web/src/components/api-keys/create-api-keys.tsx(1 hunks)web/src/components/navbar/navbar.tsx(2 hunks)web/src/components/secrets/add-secret.tsx(1 hunks)web/src/components/secrets/secret-row.tsx(1 hunks)web/src/components/secrets/secrets.tsx(2 hunks)web/src/lib/api.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- admyral/secret/secret_registry.py
- web/src/app/(workspace)/settings/api-keys/page.tsx
- web/src/app/(workspace)/settings/secrets/page.tsx
- web/src/components/api-keys/create-api-keys.tsx
🧰 Additional context used
🪛 Ruff
admyral/server/endpoints/secret_endpoints.py
28-28: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
36-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
61-61: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (16)
admyral/models/secret.py (2)
2-2: LGTM: Import follows PEP 8 guidelines
The datetime import is correctly placed and necessary for the new timestamp fields.
21-24: 🛠️ Refactor suggestion
Enhance field validation and handling
- The
emailfield should include email format validation - Consider automating timestamp management
- Verify database schema updates for these new fields
- email: str
+ email: str = Field(..., pattern=r"^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$")
- created_at: datetime
- updated_at: datetime
+ created_at: datetime = Field(default_factory=datetime.utcnow)
+ updated_at: datetime = Field(default_factory=datetime.utcnow)admyral/db/schemas/secrets_schemas.py (2)
Line range hint 14-27: LGTM! Well-structured schema configuration.
The table configuration with cascade delete ensures proper cleanup of secrets when a user is deleted.
Line range hint 44-52: Verify impact of removed to_metadata method.
The method has been updated correctly to include the secret_type field. However, we should verify that the removal of the to_metadata method doesn't break existing code.
✅ Verification successful
Let me gather more context about the SecretMetadata model and its usage.
Let me check how the SecretMetadata is being constructed in the codebase since we don't see it being used in secrets_schemas.py.
No impact from removed to_metadata method
The removal of to_metadata method is safe because:
- The
SecretMetadatamodel is now directly constructed inadmyral_store.pyinstead of using the schema'sto_metadatamethod - There are no remaining references to
SecretsSchema.to_metadata()in the codebase - All other
to_metadatausages found are from different classes (Action, Workflow, WorkflowRun) and are unrelated to this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the removed to_metadata method
# that might cause runtime errors.
echo "Searching for potential references to removed to_metadata method..."
rg "to_metadata" --type py
echo "Searching for SecretMetadata usage that might be affected..."
rg "SecretMetadata" --type py
Length of output: 3099
Script:
#!/bin/bash
# Check the SecretMetadata model definition and its usage in secrets_schemas.py
echo "Checking SecretMetadata model definition..."
rg -A 5 "class SecretMetadata" admyral/models/secret.py
echo "Checking secrets_schemas.py for SecretMetadata usage..."
cat admyral/db/schemas/secrets_schemas.py
Length of output: 2112
Script:
#!/bin/bash
# Check how SecretMetadata is being constructed in admyral_store.py
echo "Checking SecretMetadata construction in admyral_store.py..."
rg -B 2 -A 10 "SecretMetadata\(" admyral/db/admyral_store.py
# Check if there are any other to_metadata methods in secrets_schemas.py history
echo "Checking git history for to_metadata in secrets_schemas.py..."
git log -p admyral/db/schemas/secrets_schemas.py | grep -A 5 -B 5 "to_metadata"
Length of output: 2076
web/src/components/secrets/secrets.tsx (2)
4-4: LGTM! Good choice of UI library.
The addition of Radix UI's Table component is a good choice as it provides accessible and well-structured table components out of the box.
16-16: Verify the removal of store functions.
The simplification of the store usage looks good, but let's verify that the removed functions (addNewSecret and getNumberOfSecrets) aren't used elsewhere in the codebase.
admyral/server/endpoints/secret_endpoints.py (1)
34-39: LGTM with previous review comments.
The endpoint implementation looks good, but please address the validation and concurrency concerns raised in the previous review.
🧰 Tools
🪛 Ruff
36-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
web/src/components/navbar/navbar.tsx (1)
64-64: LGTM! Good security practice for external links.
Adding rel="noopener noreferrer" to external links that open in new tabs is a recommended security practice. This prevents potential security vulnerabilities where malicious websites could manipulate the original window through the window.opener property.
Also applies to: 86-86
admyral/db/store_interface.py (6)
48-48: LGTM! Modern type hint syntax.
The update to use the | syntax for union types is a good modernization that maintains the same semantic meaning.
63-63: LGTM! Consistent type hint modernization.
The updates to use | syntax for optional types across all methods are consistent and maintain the same semantic meaning while adopting modern Python type hint syntax.
Also applies to: 76-76, 94-94, 99-99, 115-115, 129-129, 132-132, 166-166, 176-176
230-232: LGTM! Enhanced secret filtering capability.
The addition of optional secret_type parameter enables better organization and filtering of secrets.
240-246: LGTM! Improved secret validation and metadata.
Good improvements:
- Added schema validation through
secret_schema - Added optional type categorization
- Return type change provides more information through
SecretMetadata
251-260: LGTM! Atomic secret updates prevent race conditions.
The new compare_and_swap_secret method is a great addition that:
- Implements atomic updates to prevent race conditions
- Maintains consistency with
store_secretparameters - Returns metadata about the operation
Let's verify the implementation in the concrete store:
✅ Verification successful
Implementation correctly ensures atomic updates with proper race condition prevention
The concrete implementation in admyral_store.py properly implements atomic updates by:
- Verifying the current secret value matches the expected value before updating
- Using a single SQL UPDATE operation with WHERE conditions
- Throwing an error if concurrent modifications are detected
- Returning metadata about the operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation to ensure atomic updates
ast-grep --pattern 'async def compare_and_swap_secret($$$): $$$'
Length of output: 66
Script:
#!/bin/bash
# Let's try a broader search for implementations using ripgrep
rg -l "compare_and_swap_secret" --type py
# Then let's look at the content of any files that implement this method
rg "compare_and_swap_secret" --type py -A 10
Length of output: 2254
Script:
#!/bin/bash
# Let's look at the complete implementation in admyral_store.py
rg "async def compare_and_swap_secret" admyral/db/admyral_store.py -A 30
Length of output: 1410
34-36: LGTM! Verify cascade deletion behavior.
The new delete_user method is well-placed and properly defined. However, since this is a deletion operation, we should verify its cascade behavior.
Let's check for any potential foreign key relationships:
✅ Verification successful
LGTM! Cascade deletion is properly handled.
Based on the search results, the database schema shows that all related entities (secrets, workflows, API keys, etc.) have proper cascade delete constraints through foreign key relationships with the User table. Specifically:
- API keys:
ForeignKeyConstraint(["user_id"], ["User.id"], ondelete="CASCADE") - Secrets: Foreign key with
ondelete="CASCADE" - Workflows: Foreign key with
ondelete="CASCADE" - Workflow schedules: Cascaded through workflow deletion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to user_id across the codebase to identify potential cascade dependencies
rg -l "user_id.*foreign\s+key|references.*users?"
Length of output: 51
Script:
#!/bin/bash
# Let's try a broader search to understand the database schema and relationships
# First, let's look for SQL schema definitions
rg -i "create\s+table.*user" -A 10
# Also search for potential relationships in Python code
rg "user_id.*=|user_id.*:" -A 2
# Look for potential cascade delete implementations
ast-grep --pattern 'async def delete_user($$$) { $$ }'
Length of output: 49278
web/src/components/secrets/add-secret.tsx (1)
25-33: LGTM! Well-structured validation helpers.
The helper functions are pure, focused, and use efficient implementations for their validation tasks.
admyral/db/admyral_store.py (1)
151-156: Verify cascade delete configuration in database schema.
The implementation assumes that associated data will be deleted via cascade. Let's verify this assumption.
✅ Verification successful
Cascade delete configuration is properly implemented
The schema definitions show that all user-related entities have cascade delete properly configured:
- User sessions
- Authenticators
- Workflows (and their runs, webhooks, schedules)
- API keys
- Secrets
All relationships are configured with sa_relationship_kwargs=dict(cascade="all, delete"), ensuring proper cleanup of all associated data when a user is deleted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if cascade delete is properly configured for UserSchema relationships
# Search for cascade configuration in schema definitions
rg -A 5 "ForeignKey.*user_id.*cascade"
rg -A 5 "relationship.*cascade.*delete"
Length of output: 3214
|
|
||
|
|
||
| class Secret(BaseModel): | ||
| secret_id: str | ||
| secret: dict[str, str] | ||
| secret_type: str | None = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent default values between Secret and EncryptedSecret
The secret_type field has a default value of None in Secret but not in EncryptedSecret. Consider making this consistent across both classes.
# In EncryptedSecret class:
- secret_type: str | None
+ secret_type: str | None = NoneCommittable suggestion skipped: line range outside the PR's diff.
| def handle_secret_values(secret_key_values: dict[str, str]) -> dict[str, str]: | ||
| return dict( | ||
| [ | ||
| (key, val.encode("utf-8").decode("unicode_escape")) | ||
| for key, val in secret_key_values.items() | ||
| ] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to secret value processing
The handle_secret_values function could fail with malformed Unicode input. Consider adding try-catch blocks to handle encoding/decoding errors gracefully.
def handle_secret_values(secret_key_values: dict[str, str]) -> dict[str, str]:
+ def safe_decode(val: str) -> str:
+ try:
+ return val.encode("utf-8").decode("unicode_escape")
+ except UnicodeError as e:
+ raise ValueError(f"Invalid secret value format: {e}")
+
return dict(
[
- (key, val.encode("utf-8").decode("unicode_escape"))
+ (key, safe_decode(val))
for key, val in secret_key_values.items()
]
)📝 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.
| def handle_secret_values(secret_key_values: dict[str, str]) -> dict[str, str]: | |
| return dict( | |
| [ | |
| (key, val.encode("utf-8").decode("unicode_escape")) | |
| for key, val in secret_key_values.items() | |
| ] | |
| ) | |
| def handle_secret_values(secret_key_values: dict[str, str]) -> dict[str, str]: | |
| def safe_decode(val: str) -> str: | |
| try: | |
| return val.encode("utf-8").decode("unicode_escape") | |
| except UnicodeError as e: | |
| raise ValueError(f"Invalid secret value format: {e}") | |
| return dict( | |
| [ | |
| (key, safe_decode(val)) | |
| for key, val in secret_key_values.items() | |
| ] | |
| ) |
admyral/secret/secrets_manager.py
Outdated
| encrypted_secret = await self.db.get_secret(user_id, delta_secret.secret_id) | ||
| if not encrypt_secret: | ||
| raise ValueError(f"Secret {delta_secret.secret_id} does not exist.") |
There was a problem hiding this comment.
Fix critical typo in existence check.
The condition uses encrypt_secret instead of encrypted_secret, which would always evaluate to False.
Apply this fix:
encrypted_secret = await self.db.get_secret(user_id, delta_secret.secret_id)
- if not encrypt_secret:
+ if not encrypted_secret:
raise ValueError(f"Secret {delta_secret.secret_id} does not exist.")📝 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.
| encrypted_secret = await self.db.get_secret(user_id, delta_secret.secret_id) | |
| if not encrypt_secret: | |
| raise ValueError(f"Secret {delta_secret.secret_id} does not exist.") | |
| encrypted_secret = await self.db.get_secret(user_id, delta_secret.secret_id) | |
| if not encrypted_secret: | |
| raise ValueError(f"Secret {delta_secret.secret_id} does not exist.") |
admyral/secret/secrets_manager.py
Outdated
| async def set(self, user_id: str, secret: Secret) -> SecretMetadata: | ||
| encrypted_secret = self._encrypt_secret(secret.secret) | ||
| secret_schema = list(secret.secret.keys()) | ||
| return await self.db.store_secret( | ||
| user_id, secret.secret_id, encrypted_secret, secret_schema | ||
| user_id, | ||
| secret.secret_id, | ||
| encrypted_secret, | ||
| secret_schema, | ||
| secret.secret_type, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add schema validation in set method.
The set method should validate the schema for registered secret types, similar to the update method.
Add validation:
async def set(self, user_id: str, secret: Secret) -> SecretMetadata:
+ if SecretRegistry.is_registered(secret.secret_type):
+ SecretRegistry.validate_schema(secret.secret_type, secret.secret)
encrypted_secret = self._encrypt_secret(secret.secret)
secret_schema = list(secret.secret.keys())📝 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.
| async def set(self, user_id: str, secret: Secret) -> SecretMetadata: | |
| encrypted_secret = self._encrypt_secret(secret.secret) | |
| secret_schema = list(secret.secret.keys()) | |
| return await self.db.store_secret( | |
| user_id, secret.secret_id, encrypted_secret, secret_schema | |
| user_id, | |
| secret.secret_id, | |
| encrypted_secret, | |
| secret_schema, | |
| secret.secret_type, | |
| async def set(self, user_id: str, secret: Secret) -> SecretMetadata: | |
| if SecretRegistry.is_registered(secret.secret_type): | |
| SecretRegistry.validate_schema(secret.secret_type, secret.secret) | |
| encrypted_secret = self._encrypt_secret(secret.secret) | |
| secret_schema = list(secret.secret.keys()) | |
| return await self.db.store_secret( | |
| user_id, | |
| secret.secret_id, | |
| encrypted_secret, | |
| secret_schema, | |
| secret.secret_type, |
| for key, value in delta_secret.secret.items(): | ||
| # ignore empty values - empty values in the delta mean that no | ||
| # update was provided for the key-value pair. | ||
| if is_empty(value): | ||
| continue | ||
| secret[key] = value |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type validation for secret values.
The update logic should validate that new values maintain consistent types with existing values to prevent type confusion attacks.
Add type validation:
for key, value in delta_secret.secret.items():
if is_empty(value):
continue
+ if key in secret and not isinstance(value, type(secret[key])):
+ raise ValueError(f"Type mismatch for key '{key}': expected {type(secret[key]).__name__}, got {type(value).__name__}")
secret[key] = value📝 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.
| for key, value in delta_secret.secret.items(): | |
| # ignore empty values - empty values in the delta mean that no | |
| # update was provided for the key-value pair. | |
| if is_empty(value): | |
| continue | |
| secret[key] = value | |
| for key, value in delta_secret.secret.items(): | |
| # ignore empty values - empty values in the delta mean that no | |
| # update was provided for the key-value pair. | |
| if is_empty(value): | |
| continue | |
| if key in secret and not isinstance(value, type(secret[key])): | |
| raise ValueError(f"Type mismatch for key '{key}': expected {type(secret[key]).__name__}, got {type(value).__name__}") | |
| secret[key] = value |
admyral/secret/secrets_manager.py
Outdated
| async def update(self, user_id: str, delta_secret: Secret) -> SecretMetadata: | ||
| if SecretRegistry.is_registered(delta_secret.secret_type): | ||
| SecretRegistry.validate_schema( | ||
| delta_secret.secret_type, delta_secret.secret | ||
| ) | ||
|
|
||
| encrypted_secret = await self.db.get_secret(user_id, delta_secret.secret_id) | ||
| if not encrypt_secret: | ||
| raise ValueError(f"Secret {delta_secret.secret_id} does not exist.") | ||
|
|
||
| if encrypted_secret.secret_type != delta_secret.secret_type: | ||
| raise ValueError("Secret type cannot be changed.") | ||
|
|
||
| secret = json.loads(decrypt_secret(encrypted_secret.encrypted_secret)) | ||
|
|
||
| for key, value in delta_secret.secret.items(): | ||
| # ignore empty values - empty values in the delta mean that no | ||
| # update was provided for the key-value pair. | ||
| if is_empty(value): | ||
| continue | ||
| secret[key] = value | ||
|
|
||
| # remove the keys which are not present | ||
| # we only allow editing the schema if the secret type is not valid, i.e., | ||
| # the secret is a custom secret. | ||
| if not SecretRegistry.is_registered(delta_secret.secret_type): | ||
| removal_keys = set(secret.keys()) - set(delta_secret.secret.keys()) | ||
| for key in removal_keys: | ||
| secret.pop(key) | ||
|
|
||
| secret_schema = list(secret.keys()) | ||
| updated_encrypted_secret = self._encrypt_secret(secret) | ||
|
|
||
| return await self.db.compare_and_swap_secret( | ||
| user_id, | ||
| delta_secret.secret_id, | ||
| encrypted_secret.encrypted_secret, | ||
| updated_encrypted_secret, | ||
| secret_schema, | ||
| delta_secret.secret_type, | ||
| ) |
There was a problem hiding this comment.
Add error handling for crypto operations.
The update method should handle potential encryption/decryption failures gracefully.
Wrap crypto operations in try-except blocks:
try:
secret = json.loads(decrypt_secret(encrypted_secret.encrypted_secret))
+ except (ValueError, DecryptionError) as e:
+ raise ValueError(f"Failed to decrypt existing secret: {str(e)}")
# ... update logic ...
try:
updated_encrypted_secret = self._encrypt_secret(secret)
+ except EncryptionError as e:
+ raise ValueError(f"Failed to encrypt updated secret: {str(e)}")Committable suggestion skipped: line range outside the PR's diff.
| useEffect(() => { | ||
| if (error) { | ||
| errorToast( | ||
| "Failed to load secret schemas. Please reload the page.", | ||
| ); | ||
| } | ||
| }, [error]); |
There was a problem hiding this comment.
Add missing dependency to useEffect.
The errorToast function should be included in the dependency array to prevent potential stale closures.
-}, [error]);
+}, [error, errorToast]);📝 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.
| useEffect(() => { | |
| if (error) { | |
| errorToast( | |
| "Failed to load secret schemas. Please reload the page.", | |
| ); | |
| } | |
| }, [error]); | |
| useEffect(() => { | |
| if (error) { | |
| errorToast( | |
| "Failed to load secret schemas. Please reload the page.", | |
| ); | |
| } | |
| }, [error, errorToast]); |
| interface SecretRowProps { | ||
| secret: TSecretMetadata; | ||
| idx: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a unique identifier instead of array index
Using array indices as props can lead to issues with React's reconciliation process, especially when items are reordered or deleted. Consider using secret.secretId instead of idx for stable component identity.
interface SecretRowProps {
secret: TSecretMetadata;
- idx: number;
+ secretId: string;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
web/src/stores/secrets-store.ts (2)
27-31: Document the prepend behavior of addNewSecretThe use of
unshift()prepends new secrets to the start of the array. This behavior should be documented as it may affect UI implementations and user expectations.+// New secrets are added to the beginning of the list addNewSecret: (newSecret: TSecretMetadata) => set( produce((draft) => { draft.secrets.unshift(newSecret); }), ),
45-47: Consider a more readable implementation for isDuplicateSecretThe current implementation using
findIndex !== -1works correctly but could be more readable usingsome().-isDuplicateSecret: (secretId: string) => - get().secrets.findIndex((secret) => secret.secretId === secretId) !== - -1, +isDuplicateSecret: (secretId: string) => + get().secrets.some((secret) => secret.secretId === secretId),web/src/components/secrets/secrets.tsx (3)
37-39: Document the purpose of empty columns.The empty columns at the start and end of the table lack documentation explaining their purpose. Consider adding comments to clarify their intended use (e.g., if they're for checkboxes, actions, etc.).
Also applies to: 52-54
34-67: Consider improving table responsiveness.The current implementation uses fixed widths for columns which might not work well on different screen sizes.
Consider:
- Using relative units or responsive design patterns
- Implementing horizontal scrolling for smaller screens
- Using CSS Grid or flexbox for better responsiveness
- <Table.ColumnHeaderCell style={{ width: "30%" }}> + <Table.ColumnHeaderCell className="name-column">Add corresponding CSS:
.name-column { min-width: 200px; flex: 1; }
59-65: Consider removing the idx prop if unused.The
idxprop passed to SecretRow might be unnecessary if it's not being used for any specific purpose. Since you're already using a proper key based onsecretId, the index might be redundant.- <SecretRow - key={`secret_row_${secret.secretId}`} - secret={secret} - idx={idx} - /> + <SecretRow + key={`secret_row_${secret.secretId}`} + secret={secret} + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/src/components/secrets/secrets.tsx(2 hunks)web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx(1 hunks)web/src/stores/secrets-store.ts(3 hunks)web/src/stores/workflow-store.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx
🔇 Additional comments (7)
web/src/stores/secrets-store.ts (2)
1-1: Strong type improvements!
The transition to using only TSecretMetadata throughout the store interface significantly improves type safety and reduces potential runtime errors. The method signatures are now more precise and consistent.
Also applies to: 6-6, 8-11, 13-13
Line range hint 1-48: Verify compatibility with new secret models
The store has been updated to work with TSecretMetadata, but let's verify that this type properly accommodates all the new secret models mentioned in the summary (AnthropicSecret, AzureOpenAISecret, MistralAISecret, etc.).
✅ Verification successful
Store implementation is compatible with secret models
The TSecretMetadata type is well-designed to handle different secret models through its flexible schema:
- It includes
secret_typefield that can accommodate different provider types - The
secret_schemafield is an array of strings that can define the required fields for each provider - The actual secret structure supports key-value pairs that can map to any provider's requirements
- The secret schemas are dynamically fetched from the API (
/api/v1/secrets/schemas) rather than hardcoded, allowing for extensibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TSecretMetadata type definition and its usage with new secret models
rg -t typescript "TSecretMetadata" --glob "!*.test.*" -A 5
Length of output: 93
Script:
#!/bin/bash
# Search for TSecretMetadata type definition and usage
rg "TSecretMetadata" -A 5
# Also search for secret type/model definitions
rg "type.*Secret" -A 5
Length of output: 6561
Script:
#!/bin/bash
# Let's look at the SecretMetadata schema definition to understand the type structure
rg "SecretMetadata =" -B 2 -A 10
# Also search for any secret model/provider related schemas
rg "const.*Secret.*Schema" -A 10
Length of output: 3354
web/src/components/secrets/secrets.tsx (2)
4-4: LGTM! Good component organization.
The addition of Radix UI Table and separation of SecretRow into its own component shows good component organization and maintainability practices.
Also applies to: 8-8
Line range hint 11-32: LGTM! Clean and efficient state management.
The component demonstrates good practices:
- Proper cleanup in useEffect
- Clear error and loading state handling
- Simplified state management by removing unused functions
web/src/stores/workflow-store.ts (3)
247-249: LGTM! Efficient check for start node removal.
The change from filter to some is a good optimization since we only need to know if any node matches the condition.
Line range hint 262-269: LGTM! Proper state cleanup on node deletion.
Good handling of UI state by clearing the detail page type and selection when a node is deleted while being viewed.
Line range hint 273-286: LGTM! Efficient edge deletion tracking.
Good use of Set for tracking deleted edges and proper immutable state management with immer's produce. The implementation effectively supports the edge restoration feature when start node deletion is prevented.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (15)
admyral/db/alembic/versions/7e5d82221765_introduce_composed_primary_key_for_.py (1)
1-7: Enhance migration description with more context.Consider adding more details to the migration description about:
- The purpose of introducing a composite key
- The impact on existing data
- Any prerequisites or post-migration steps
-"""introduce composed primary key for secrets table +"""Introduce composite primary key (secret_id, user_id) for secrets table + +This migration modifies the primary key constraint of the secrets table to ensure +that secrets are uniquely identified by both secret_id and user_id, preventing +potential conflicts in multi-user scenarios. + +Prerequisites: +- Ensure no duplicate combinations of secret_id and user_id exist +- Migration db5cf73c0869 must be applied firstadmyral/db/schemas/secrets_schemas.py (3)
31-31: Remove redundant index specification on primary key field.The
index=Trueparameter is redundant since primary key columns are automatically indexed by the database.- user_id: str = Field(sa_type=TEXT(), primary_key=True, index=True) + user_id: str = Field(sa_type=TEXT(), primary_key=True)
38-39: Enhance docstring for secret_type field.The current docstring could be more descriptive to help developers understand:
- The purpose of categorizing secrets
- Expected/valid secret types
- Impact on secret handling
- """ Type of the secret. """ + """ Categorizes the type of secret (e.g., 'api_key', 'oauth_token', 'certificate'). + This field helps determine how the secret should be handled and validated. """
Line range hint
45-52: Add error handling for JSON deserialization.The
json.loads()call could raise a JSONDecodeError if schema_json_serialized contains invalid JSON. Consider adding error handling to provide a more graceful failure mode.def to_model(self, include_resources: bool = False) -> EncryptedSecret: + try: + secret_schema = json.loads(self.schema_json_serialized) + except json.JSONDecodeError as e: + raise ValueError(f"Invalid secret schema format: {e}") + return EncryptedSecret.model_validate( { "secret_id": self.secret_id, "encrypted_secret": self.encrypted_secret, - "secret_schema": json.loads(self.schema_json_serialized), + "secret_schema": secret_schema, "secret_type": self.secret_type, } )admyral/server/endpoints/secret_endpoints.py (3)
26-31: Improved API response with metadataThe endpoint now returns
SecretMetadatainstead ofNone, providing better feedback to clients.Consider extracting the
Depends(authenticate)to a module-level variable to address the Ruff B008 warning:authenticate_user = Depends(authenticate) @router.post("/set", status_code=status.HTTP_201_CREATED) async def set_secret( secret: Secret, authenticated_user: AuthenticatedUser = authenticate_user ) -> SecretMetadata:🧰 Tools
🪛 Ruff
28-28: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
59-65: Consider caching schema dataThe schema list is likely static during runtime. Consider caching the sorted result to avoid unnecessary processing on each request.
# At module level _cached_schemas: list[tuple[str, list[str]]] | None = None @router.get("/schemas") async def get_secret_schemas( _authenticated_user: AuthenticatedUser = Depends(authenticate), ) -> list[tuple[str, list[str]]]: global _cached_schemas if _cached_schemas is None: schemas = SecretRegistry.get_secret_schemas() _cached_schemas = sorted(list(schemas.items()), key=lambda x: x[0]) return _cached_schemas🧰 Tools
🪛 Ruff
61-61: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
Based on the gathered information, I can now provide the final response:
Add consistent error handling across secret endpoints
The secret endpoints currently lack explicit error handling despite the underlying
SecretsManagerimplementation throwing variousValueErrorexceptions for cases like:
- Non-existent secrets during updates
- Secret type changes
- Schema validation failures
- Invalid secret types
Add HTTP exception handling to map these errors to appropriate status codes:
- Add error handlers in
secret_endpoints.pyto catchValueErrorexceptions and return HTTP 400/404 responses, similar to how it's done in other endpoints likeworkflow_run_endpoints.pyandeditor_endpoints.py- Consider standardizing error responses across all secret operations (set, update, delete, list)
🔗 Analysis chain
Line range hint
26-65: Verify consistent error handling across endpointsEnsure consistent error handling patterns across all secret-related endpoints.
Let me gather more information about error handling in the secret endpoints specifically.
Let me check the implementation of the secrets manager to understand the error handling at the service layer.
Let me check the error handling in the database layer to complete the analysis.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in secret-related endpoints rg "HTTPException|raise\s+\w+Error" "admyral/server/endpoints/"Length of output: 1909
Script:
#!/bin/bash # Search for imports and error handling in secret endpoints rg "^from|^import|HTTPException|raise\s+\w+Error" "admyral/server/endpoints/secret_endpoints.py" -A 2Length of output: 370
Script:
#!/bin/bash # Search for error handling in secrets manager implementation rg "class.*SecretsManager|def (set|update|delete|list)|raise\s+\w+Error|HTTPException" "admyral/secret/" -A 5Length of output: 8321
Script:
#!/bin/bash # Search for error handling in the database layer for secrets rg "def (get_secret|delete_secret|list_secrets)|raise\s+\w+Error|HTTPException" "admyral/store/" -A 5Length of output: 156
🧰 Tools
🪛 Ruff
28-28: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
36-36: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
admyral/db/alembic/versions/db5cf73c0869_migrate_secrets_to_secret_type.py (2)
25-56: Standardize data structure for integration mappings.The integrations list mixes list and tuple types inconsistently. While both work for unpacking, it's better to standardize on one type for consistency and maintainability.
Apply this change:
integrations = [ - ("anthropic", "Anthropic"), - ("mistral ai", "Mistral AI"), - ("openai", "OpenAI"), - ["Jira", "Jira"], + ("Jira", "Jira"), # ... convert all list entries to tuples ]
22-68: Add logging and error handling for the migration process.The migration lacks visibility into its progress and proper error handling. This could make it difficult to debug issues in production.
Consider adding:
- Logging for each update operation
- Transaction management
- Error handling for failed updates
def upgrade() -> None: + from logging import getLogger + logger = getLogger(__name__) + + logger.info("Starting secret_type migration") + + try: + with op.get_bind().begin() as transaction: # ... existing code ... + logger.info(f"Updated secret_type for {potential_name} to {secret_type}") + except Exception as e: + logger.error(f"Migration failed: {str(e)}") + raiseadmyral/secret/secrets_manager.py (1)
131-142: Add size validation for secrets.Consider adding size validation to prevent storing excessively large secrets.
async def set(self, user_id: str, secret: Secret) -> SecretMetadata: if SecretRegistry.is_registered(secret.secret_type): SecretRegistry.validate_schema(secret.secret_type, secret.secret) + serialized_secret = json.dumps(secret.secret) + if len(serialized_secret) > 1024 * 1024: # 1MB limit + raise ValueError("Secret exceeds maximum allowed size of 1MB") + encrypted_secret = self._encrypt_secret(secret.secret) secret_schema = list(secret.secret.keys()) return await self.db.store_secret(web/src/components/secrets/add-secret.tsx (5)
25-33: Consider adding a type interface for secret key-value pairs.The helper functions could benefit from a more specific type interface rather than using an inline object type.
+interface SecretKeyValue { + key: string; + value: string; +} -function hasDuplicateSecretKeyFields( - secret: { key: string; value: string }[], +function hasDuplicateSecretKeyFields( + secret: SecretKeyValue[], ): boolean { return new Set(secret.map((kv) => kv.key)).size < secret.length; } -function hasEmptyKeyOrValue(secret: { key: string; value: string }[]): boolean { +function hasEmptyKeyOrValue(secret: SecretKeyValue[]): boolean { return secret.some((kv) => kv.key.length === 0 || kv.value.length === 0); }
38-50: Extract dialog state interface for better maintainability.Consider extracting the dialog state interface to improve code organization and reusability.
+interface SecretDialogState { + open: boolean; + secretId: string; + secretType: string | null; + secret: SecretKeyValue[]; + error: string | null; +} -const [dialogState, setDialogState] = useImmer<{ - open: boolean; - secretId: string; - secretType: string | null; - secret: { key: string; value: string }[]; - error: string | null; -}>({ +const [dialogState, setDialogState] = useImmer<SecretDialogState>({
91-96: Add type safety for API response.The
setSecret.mutateAsyncresponse type should be explicitly defined to ensure type safety throughout the component.+interface Secret { + secretId: string; + secretType: string | null; + secret: SecretKeyValue[]; +} -const newSecret = await setSecret.mutateAsync({ +const newSecret: Secret = await setSecret.mutateAsync({
122-149: Use optional chaining for better null safety.The conditional rendering of secret schemas could be simplified using optional chaining.
-{secretsSchemas && - secretsSchemas.map((secretTypeAndSchema, idx) => ( +{secretsSchemas?.map((secretTypeAndSchema, idx) => (🧰 Tools
🪛 Biome
[error] 122-149: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
213-214: Consider moving documentation URL to configuration.Hardcoded URLs should be moved to a configuration file to make them easier to maintain and update.
+// In config.ts +export const DOCS_URLS = { + integrations: 'https://docs.admyral.dev/integrations/integrations' +} as const; -href="https://docs.admyral.dev/integrations/integrations" +href={DOCS_URLS.integrations}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
admyral/actions/integrations/cdr/ms_defender_for_cloud.py(2 hunks)admyral/db/alembic/versions/7e5d82221765_introduce_composed_primary_key_for_.py(1 hunks)admyral/db/alembic/versions/db5cf73c0869_migrate_secrets_to_secret_type.py(1 hunks)admyral/db/schemas/secrets_schemas.py(3 hunks)admyral/secret/secrets_manager.py(4 hunks)admyral/server/endpoints/secret_endpoints.py(2 hunks)web/src/components/secrets/add-secret.tsx(1 hunks)web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx(2 hunks)web/src/hooks/use-get-secret-schemas-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- admyral/actions/integrations/cdr/ms_defender_for_cloud.py
- web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx
- web/src/hooks/use-get-secret-schemas-api.ts
🧰 Additional context used
🪛 Ruff
admyral/server/endpoints/secret_endpoints.py
28-28: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
36-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
61-61: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🪛 Biome
web/src/components/secrets/add-secret.tsx
[error] 122-149: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (12)
admyral/db/alembic/versions/7e5d82221765_introduce_composed_primary_key_for_.py (1)
22-31: Verify data integrity before migration.
The upgrade operation could fail if there are duplicate combinations of secret_id and user_id. Additionally, consider index implications.
Let's verify the data integrity:
Consider adding these safety checks to the migration:
def upgrade() -> None:
# ### commands auto generated by Alembic - please adjust! ###
+ # Verify no duplicate combinations exist
+ op.execute("""
+ SELECT secret_id, user_id, COUNT(*)
+ FROM secrets
+ GROUP BY secret_id, user_id
+ HAVING COUNT(*) > 1
+ """)
+
op.execute("""
ALTER TABLE secrets
DROP CONSTRAINT secrets_pkey,
ADD PRIMARY KEY (secret_id, user_id)
""")
+
+ # Consider adding an index on user_id if frequent queries by user
+ op.execute("""
+ CREATE INDEX IF NOT EXISTS idx_secrets_user_id ON secrets(user_id)
+ """)✅ Verification successful
Migration is safe to proceed
Based on the migration history, this change is safe because:
- The
secretstable was created withuser_idcolumn from the beginning (in migration014da66fd11f) - A foreign key was added to link
user_idtoUser.id(in migration552e7a8dec07) - The foreign key constraint ensures data integrity with the
Usertable - An index on
user_idalready exists (ix_secrets_user_id)
The existing index and foreign key constraint implicitly ensure there won't be any orphaned or invalid user_id values, making this composite primary key migration safe to proceed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential data conflicts before migration
# Test 1: Check for duplicate combinations that would violate the new PK
echo "Checking for duplicate secret_id and user_id combinations..."
ast-grep --pattern 'CREATE TABLE secrets' -A 10
rg -A 5 "CREATE TABLE.*secrets"
# Test 2: Check if there are any existing indexes on these columns
echo "Checking for existing indexes..."
rg -A 5 "CREATE.*INDEX.*secrets"
Length of output: 324
Script:
#!/bin/bash
# Let's try a different approach to find table definition and indexes
# Search for table definition in all migration files
echo "=== Searching for secrets table definition ==="
fd --extension py . admyral/db/alembic/versions/ --exec grep -l "secrets" {} \; | xargs cat
# Search for any index creation on secrets table in migration files
echo -e "\n=== Searching for index creation statements ==="
rg "CREATE.*INDEX.*secrets|ADD.*INDEX.*secrets" admyral/db/alembic/versions/
# Search for table definition in models
echo -e "\n=== Searching for table definition in models ==="
fd --extension py . admyral/db/ --exec grep -l "Table.*secrets\|secrets.*Table" {} \; | xargs cat
Length of output: 20367
admyral/db/schemas/secrets_schemas.py (1)
34-38: Consider adding validation constraints for sensitive fields.
For enhanced security:
- Add validation to ensure encrypted_secret is only null in specific, documented scenarios
- Consider using an Enum for secret_type to enforce valid categories
- Add validation to ensure schema_json_serialized contains expected structure
Let's check if there are any validation constraints defined elsewhere:
admyral/server/endpoints/secret_endpoints.py (2)
14-20: Add error handling for malformed secret values
The handle_secret_values function needs defensive error handling for malformed Unicode input.
The previous review comment suggesting error handling improvements is still valid and should be addressed.
34-39: Add validation for secret existence
The update endpoint needs validation to ensure the secret exists before attempting an update.
The previous review comment suggesting validation and optimistic locking is still valid and should be addressed.
🧰 Tools
🪛 Ruff
36-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
admyral/db/alembic/versions/db5cf73c0869_migrate_secrets_to_secret_type.py (1)
1-20: LGTM: Migration metadata is well-structured.
The migration metadata is properly defined with clear revision IDs and dependencies.
admyral/secret/secrets_manager.py (7)
8-9: LGTM: Import changes are appropriate.
The new imports support the added functionality for secret updates and validation.
Line range hint 75-84: LGTM: Get method implementation is correct.
The implementation properly handles the case when no secret exists and uses the new return type annotation.
86-87: LGTM: Good encapsulation of encryption logic.
The _encrypt_secret helper method improves code reuse and maintainability.
102-109: Skip generating comment: Past review comment about type validation is still valid.
89-100: 🛠️ Refactor suggestion
Add error handling for decryption failures.
While the validation logic is good, we should handle potential decryption failures gracefully.
encrypted_secret = await self.db.get_secret(user_id, delta_secret.secret_id)
if not encrypted_secret:
raise ValueError(f"Secret {delta_secret.secret_id} does not exist.")
+ try:
if encrypted_secret.secret_type != delta_secret.secret_type:
raise ValueError("Secret type cannot be changed.")
+ except Exception as e:
+ raise ValueError(f"Failed to validate secret type: {str(e)}")Likely invalid or redundant comment.
38-38: Verify impact of breaking change in set method.
The return type change from None to SecretMetadata is a breaking change that might affect existing clients.
Let's check for usages:
✅ Verification successful
Let's try a broader search to find any potential usages or implementations of the set method in the codebase.
Breaking change impact is minimal and already handled
The return type change from None to SecretMetadata in the set method has limited impact:
- Only one direct usage found in
secret_endpoints.pywhich already expects and returns theSecretMetadataresult - No inheritance implementations of
SecretsManagerclass were found that override thesetmethod - Other
.set()calls found in the codebase are unrelated to theSecretsManagerclass
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find direct calls to set method
ast-grep --pattern 'secrets_manager.set($$$)'
# Find method overrides
ast-grep --pattern 'class $_ (SecretsManager):
$$$
async def set($$$):
$$$'
Length of output: 133
Script:
#!/bin/bash
# Search for SecretsManager class usage and inheritance
rg -l "SecretsManager"
# Search for method calls using ripgrep
rg "\.set\(" -A 2
# Search for async method calls
rg "await.*\.set\(" -A 2
# Look for test files that might use the set method
fd "test.*\.py$" --exec rg "set\(" {} -A 2
Length of output: 1383
14-14: LGTM: Method signatures and documentation are well-defined.
The use of | syntax for union types implies a Python 3.10+ requirement. The update method's documentation clearly explains its behavior, particularly regarding empty value handling.
Let's verify the Python version requirement is documented:
Also applies to: 25-35
| def downgrade() -> None: | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
|
|
||
| op.execute(""" | ||
| ALTER TABLE secrets | ||
| DROP CONSTRAINT secrets_pkey, | ||
| ADD PRIMARY KEY (secret_id) | ||
| """) | ||
|
|
||
| # ### end Alembic commands ### |
There was a problem hiding this comment.
Critical: Prevent data loss in downgrade operation.
The downgrade operation could lead to data loss if multiple secrets exist with the same secret_id but different user_id values, as PostgreSQL will silently keep only one row when enforcing the new single-column primary key.
Add data validation and handling:
def downgrade() -> None:
# ### commands auto generated by Alembic - please adjust! ###
+ # Check for potential conflicts
+ op.execute("""
+ WITH duplicates AS (
+ SELECT secret_id, COUNT(*)
+ FROM secrets
+ GROUP BY secret_id
+ HAVING COUNT(*) > 1
+ )
+ SELECT CASE
+ WHEN EXISTS (SELECT 1 FROM duplicates)
+ THEN RAISE EXCEPTION 'Cannot downgrade: Multiple secrets exist with the same secret_id'
+ END;
+ """)
op.execute("""
ALTER TABLE secrets
DROP CONSTRAINT secrets_pkey,
ADD PRIMARY KEY (secret_id)
""")📝 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.
| def downgrade() -> None: | |
| # ### commands auto generated by Alembic - please adjust! ### | |
| op.execute(""" | |
| ALTER TABLE secrets | |
| DROP CONSTRAINT secrets_pkey, | |
| ADD PRIMARY KEY (secret_id) | |
| """) | |
| # ### end Alembic commands ### | |
| def downgrade() -> None: | |
| # ### commands auto generated by Alembic - please adjust! ### | |
| # Check for potential conflicts | |
| op.execute(""" | |
| WITH duplicates AS ( | |
| SELECT secret_id, COUNT(*) | |
| FROM secrets | |
| GROUP BY secret_id | |
| HAVING COUNT(*) > 1 | |
| ) | |
| SELECT CASE | |
| WHEN EXISTS (SELECT 1 FROM duplicates) | |
| THEN RAISE EXCEPTION 'Cannot downgrade: Multiple secrets exist with the same secret_id' | |
| END; | |
| """) | |
| op.execute(""" | |
| ALTER TABLE secrets | |
| DROP CONSTRAINT secrets_pkey, | |
| ADD PRIMARY KEY (secret_id) | |
| """) | |
| # ### end Alembic commands ### |
| def downgrade() -> None: | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| pass | ||
| # ### end Alembic commands ### |
There was a problem hiding this comment.
Implement downgrade functionality for rollback safety.
The empty downgrade function provides no way to rollback changes if needed. This is risky for production deployments.
Consider implementing a downgrade strategy:
def downgrade() -> None:
- pass
+ # Store original values in a temporary table during upgrade
+ # Restore from the temporary table during downgrade
+ op.execute("""
+ UPDATE secrets s
+ SET secret_type = t.original_type
+ FROM temp_secret_types t
+ WHERE s.id = t.secret_id
+ """)Would you like me to help create a more detailed downgrade implementation that includes creating and managing a temporary table?
Committable suggestion skipped: line range outside the PR's diff.
| for potential_name, secret_type in integrations: | ||
| potential_name: str = potential_name.lower() | ||
| op.execute( | ||
| f"UPDATE secrets SET secret_type = '{secret_type}' WHERE secret_type = '%{potential_name}%'" | ||
| ) | ||
| potential_name = potential_name.replace(" ", "_") | ||
| op.execute( | ||
| f"UPDATE secrets SET secret_type = '{secret_type}' WHERE secret_type = '%{potential_name}%'" | ||
| ) |
There was a problem hiding this comment.
Critical: Fix SQL injection vulnerability and LIKE syntax.
The current implementation has several critical issues:
- Using f-strings with SQL queries is vulnerable to SQL injection
- The LIKE syntax is incorrect (using '%value%' instead of 'value')
- No error handling for failed updates
Replace the current implementation with parameterized queries:
for potential_name, secret_type in integrations:
potential_name: str = potential_name.lower()
- op.execute(
- f"UPDATE secrets SET secret_type = '{secret_type}' WHERE secret_type = '%{potential_name}%'"
- )
+ op.execute(
+ "UPDATE secrets SET secret_type = :new_type WHERE secret_type LIKE :pattern",
+ {"new_type": secret_type, "pattern": f"%{potential_name}%"}
+ )
potential_name = potential_name.replace(" ", "_")
- op.execute(
- f"UPDATE secrets SET secret_type = '{secret_type}' WHERE secret_type = '%{potential_name}%'"
- )
+ op.execute(
+ "UPDATE secrets SET secret_type = :new_type WHERE secret_type LIKE :pattern",
+ {"new_type": secret_type, "pattern": f"%{potential_name}%"}
+ )📝 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.
| for potential_name, secret_type in integrations: | |
| potential_name: str = potential_name.lower() | |
| op.execute( | |
| f"UPDATE secrets SET secret_type = '{secret_type}' WHERE secret_type = '%{potential_name}%'" | |
| ) | |
| potential_name = potential_name.replace(" ", "_") | |
| op.execute( | |
| f"UPDATE secrets SET secret_type = '{secret_type}' WHERE secret_type = '%{potential_name}%'" | |
| ) | |
| for potential_name, secret_type in integrations: | |
| potential_name: str = potential_name.lower() | |
| op.execute( | |
| "UPDATE secrets SET secret_type = :new_type WHERE secret_type LIKE :pattern", | |
| {"new_type": secret_type, "pattern": f"%{potential_name}%"} | |
| ) | |
| potential_name = potential_name.replace(" ", "_") | |
| op.execute( | |
| "UPDATE secrets SET secret_type = :new_type WHERE secret_type LIKE :pattern", | |
| {"new_type": secret_type, "pattern": f"%{potential_name}%"} | |
| ) |
| # remove the keys which are not present | ||
| # we only allow editing the schema if the secret type is not valid, i.e., | ||
| # the secret is a custom secret. | ||
| if not SecretRegistry.is_registered(delta_secret.secret_type): | ||
| removal_keys = set(secret.keys()) - set(delta_secret.secret.keys()) | ||
| for key in removal_keys: | ||
| secret.pop(key) | ||
|
|
||
| secret_schema = list(secret.keys()) | ||
| updated_encrypted_secret = self._encrypt_secret(secret) | ||
|
|
||
| return await self.db.compare_and_swap_secret( | ||
| user_id, | ||
| delta_secret.secret_id, | ||
| encrypted_secret.encrypted_secret, | ||
| updated_encrypted_secret, | ||
| secret_schema, | ||
| delta_secret.secret_type, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction to prevent race conditions in compare-and-swap.
The compare-and-swap operation should be wrapped in a transaction to prevent race conditions.
+ async with self.db.transaction():
secret_schema = list(secret.keys())
updated_encrypted_secret = self._encrypt_secret(secret)
return await self.db.compare_and_swap_secret(
user_id,
delta_secret.secret_id,
encrypted_secret.encrypted_secret,
updated_encrypted_secret,
secret_schema,
delta_secret.secret_type,
)📝 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.
| # remove the keys which are not present | |
| # we only allow editing the schema if the secret type is not valid, i.e., | |
| # the secret is a custom secret. | |
| if not SecretRegistry.is_registered(delta_secret.secret_type): | |
| removal_keys = set(secret.keys()) - set(delta_secret.secret.keys()) | |
| for key in removal_keys: | |
| secret.pop(key) | |
| secret_schema = list(secret.keys()) | |
| updated_encrypted_secret = self._encrypt_secret(secret) | |
| return await self.db.compare_and_swap_secret( | |
| user_id, | |
| delta_secret.secret_id, | |
| encrypted_secret.encrypted_secret, | |
| updated_encrypted_secret, | |
| secret_schema, | |
| delta_secret.secret_type, | |
| ) | |
| # remove the keys which are not present | |
| # we only allow editing the schema if the secret type is not valid, i.e., | |
| # the secret is a custom secret. | |
| if not SecretRegistry.is_registered(delta_secret.secret_type): | |
| removal_keys = set(secret.keys()) - set(delta_secret.secret.keys()) | |
| for key in removal_keys: | |
| secret.pop(key) | |
| async with self.db.transaction(): | |
| secret_schema = list(secret.keys()) | |
| updated_encrypted_secret = self._encrypt_secret(secret) | |
| return await self.db.compare_and_swap_secret( | |
| user_id, | |
| delta_secret.secret_id, | |
| encrypted_secret.encrypted_secret, | |
| updated_encrypted_secret, | |
| secret_schema, | |
| delta_secret.secret_type, | |
| ) |
danielgrittner
force-pushed
the
rework-settings
branch
from
e63c255 to
fa3fa1f
Compare
What does this PR do?
Fixes # (issue)
Type of change
Mandatory Tasks
Checklist
npm run prettierpnpm prettiercargo fmtnpm run lint)poetry run pytestcargo testSummary by CodeRabbit
Release Notes
New Features
AnthropicSecret,AzureOpenAISecret,MistralAISecret, etc.) to enhance API integrations with structured secret validation.AccountPagecomponent for user account management.DeleteAccountButtoncomponent for user account deletion.SecretsPagecomponent for managing secrets with enhanced UI.SideNavSettingscomponent for navigation within settings.Improvements
NavLinkcomponent for better selection logic.SecretsStoretype for stricter type safety.Bug Fixes
Chores