fix: Init target variable ui #171
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several new React components for managing target variables, including dialogs for creating, editing, and deleting target variables. It modifies existing components to accommodate these changes, enhancing the user interface for variable management. Additionally, backend updates include a new router for target variables, modifications to database schemas to establish relationships, and updates to validation and type handling for target variables. Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (3)
packages/db/src/schema/target-provider.ts (1)
Line range hint
1-50: Well-structured schema with proper relationships and constraints.The schema design follows database best practices:
- Uses UUIDs for primary keys
- Implements proper foreign key constraints with cascade deletes
- Ensures uniqueness of provider names within workspaces
- Maintains audit trails with timestamps
- Provides type safety through zod schemas
Consider adding indexes on foreign key columns (
workspaceId,targetProviderId) if they're frequently used in queries.apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/VariablesContent.tsx (1)
73-102: Consider adding tooltips for better UX.The variable row implementation is solid, but could benefit from tooltips to explain the lock icon and variable type indicators to users.
Here's a suggested enhancement:
<div className="flex items-center gap-2"> <span>{varKey} </span> - {sensitive && <IconLock className="h-4 w-4 text-muted-foreground" />} + {sensitive && ( + <span title="This variable contains sensitive data"> + <IconLock className="h-4 w-4 text-muted-foreground" /> + </span> + )} </div>apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/EditTargetVariableDialog.tsx (1)
218-225: Associate the label with the checkbox for improved accessibility.The
Checkboxcomponent lacks anidattribute, so thelabel'shtmlForattribute does not correctly reference it. Adding anidto theCheckboxensures the label is properly associated.Apply this diff to fix the issue:
<Checkbox + id="sensitive" checked={value} onCheckedChange={onSensitiveChange} /> <label htmlFor="sensitive" className="text-sm"> Sensitive </label>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/DeleteTargetVariableDialog.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/EditTargetVariableDialog.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetVariableDropdown.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/VariablesContent.tsx (2 hunks)
- packages/api/package.json (1 hunks)
- packages/api/src/router/target.ts (3 hunks)
- packages/db/src/schema/target-provider.ts (3 hunks)
- packages/db/src/schema/target.ts (4 hunks)
🔇 Additional comments (19)
packages/db/src/schema/target-provider.ts (2)
2-2: LGTM: Import statements are properly structured.The new imports are necessary for establishing the relationship between target provider and target tables.
Also applies to: 13-13
29-32: Verify the corresponding relationship in the target table.The one-to-many relationship definition looks correct. However, let's verify that the target table has the corresponding many-to-one relationship defined.
✅ Verification successful
Based on the search results, I can see that the target table does have the corresponding many-to-one relationship with targetProvider correctly defined. This is visible in the target schema definition at
packages/db/src/schema/target.tswhere it shows:export const targetRelations = relations(target, ({ one, many }) => ({ metadata: many(targetMetadata), variables: many(targetVariable), provider: one(targetProvider, { fields: [target.providerId], references: [targetProvider.id], }), }));The relationship is properly defined in both directions
The target table has a many-to-one relationship with targetProvider through the
providerfield, which correctly corresponds to the one-to-many relationship defined in the target-provider table. The foreign key and references are properly set up.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the corresponding relationship in the target table # Expected: Should find a many-to-one relationship with targetProvider in the target table # Search for relationship definition in target.ts ast-grep --pattern 'relations($_, ({ one }) => ({ targetProvider: one($_) }))'Length of output: 80
Script:
#!/bin/bash # Let's try a broader search to find any relations definition in target-related files fd "target.*\.ts" --exec ast-grep --pattern 'relations($_, $_)' # Also search for the target table definition to understand the schema fd "target.*\.ts" --exec cat {}Length of output: 184961
packages/api/package.json (1)
26-26: LGTM! Good practice using a dedicated secrets package.The addition of
@ctrlplane/secretsas a workspace dependency is well-placed and follows the established pattern for internal package dependencies.apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/DeleteTargetVariableDialog.tsx (2)
1-22: LGTM! Clean imports and well-defined TypeScript types.The imports are properly organized and the props interface is well-defined with clear type annotations.
24-26: Verify user permissions for deletion.Consider adding permission checks before allowing the deletion operation to proceed.
✅ Verification successful
Let me search for more context about the permission handling in the API layer.
Let me search specifically for the target variable router and its middleware setup.
Let me check the target router implementation specifically.
Permission checks are properly implemented
The
DeleteTargetVariableDialogcomponent uses thetarget.variable.deletemutation which is properly protected with authorization checks. The router implementation shows that the delete mutation is wrapped withprotectedProcedureand includes an authorization check that verifies the user hasTargetUpdatepermission on the specific target before allowing the deletion.Relevant implementation from
targetRouter:delete: protectedProcedure .input(z.string().uuid()) .meta({ authorizationCheck: async ({ ctx, canUser, input }) => { const variable = await ctx.db .select() .from(schema.targetVariable) .where(eq(schema.targetVariable.id, input)) .then(takeFirstOrNull); if (!variable) return false; return canUser .perform(Permission.TargetUpdate) .on({ type: "target", id: variable.targetId }); }, })🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for permission checks in related files rg -A 5 "target\.variable\.delete"Length of output: 805
Script:
#!/bin/bash # Search for the API router definition for target variable deletion ast-grep --pattern 'publicProcedure.input($_).mutation(async ({ ctx, input }) => { $$$ })' # Also check for protected routes ast-grep --pattern 'protectedProcedure.input($_).mutation(async ({ ctx, input }) => { $$$ })' # Look for any auth middleware rg -A 10 "createTRPCRouter|protectedProcedure|publicProcedure" --type typescriptLength of output: 3122
Script:
#!/bin/bash # Search for target variable router definition rg -A 10 "variable.*delete" packages/api/src/router/ # Look for tRPC router setup and middleware rg -A 10 "router|middleware" packages/api/src/Length of output: 2598
Script:
#!/bin/bash # Look for target router implementation cat packages/api/src/router/target.ts # Also check for any auth middleware in trpc.ts cat packages/api/src/trpc.tsLength of output: 19636
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetVariableDropdown.tsx (2)
1-19: LGTM! Well-organized imports and clear type definitions.The imports are logically grouped and the type definitions are properly typed with clear prop requirements.
21-25: LGTM! Component declaration follows React+TypeScript best practices.The component is properly typed as a React.FC with appropriate prop destructuring.
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/VariablesContent.tsx (1)
Line range hint
3-19: LGTM! Import statements are well-organized.The new imports are properly structured and necessary for the added functionality.
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (2)
194-197: Verify null/undefined handling for target.variables.Ensure that
VariableContentcomponent properly handles cases wheretarget.variablesmight be undefined. Consider adding a default empty array or proper null checks.Let's check how
VariableContenthandles undefined variables:#!/bin/bash # Search for null checks in VariableContent rg -A 5 "targetVariables(\?|:)" --type typescript
194-197:⚠️ Potential issueAdd type safety for targetVariables prop.
While passing
target.variablesto theVariableContentcomponent is correct, the type annotation is currently set toanywhich bypasses TypeScript's type checking benefits. Consider defining a proper type for the variables to ensure type safety and better IDE support.Let's verify the type definition:
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx (1)
1-37: LGTM: Well-structured imports and type definitions.The imports are logically organized, and the props interface is clearly defined with appropriate types.
packages/db/src/schema/target.ts (6)
6-6: Import 'relations' from 'drizzle-orm'The addition of
relationsto the import statement is appropriate, as it is used to define table relationships in the schema.
54-61: DefinetargetRelationscorrectlyThe
targetRelationsare correctly defined, establishing relationships betweentargetandmetadata,variables, andprovider. The fields and references are properly aligned, which will facilitate accurate ORM operations.
122-127: DefinetargetMetadataRelationscorrectlyThe
targetMetadataRelationsproperly establish the one-to-one relationship betweentargetMetadataandtarget. The fields and references are correctly specified, ensuring data integrity and facilitating efficient queries.
252-257: DefinetargetVariableRelationscorrectlyThe
targetVariableRelationsare accurately defined, establishing the relationship betweentargetVariableandtarget. The fields and references are properly set, which will ensure correct relational mapping in the ORM.
259-261: UpdatecreateTargetVariableschema to match value typesThe
createTargetVariableschema now correctly specifies thatvaluecan be astring,number, orboolean, aligning with the updated type definition in the table schema. This ensures proper validation during insert operations.
246-246: Verify the type restriction onvaluefieldBy specifying the
valuefield to be of typestring | number | boolean, we restrict it to primitive data types. Ifvalueneeds to support complex data types like objects or arrays, this change might be too limiting. Please confirm that this type restriction aligns with all use cases.Run the following script to check if there are any usages of
targetVariable.valuewith complex data types:✅ Verification successful
Based on the search results, I can now verify the type restriction on the
valuefield. The codebase shows that target variables are used in various contexts, and the type restriction tostring | number | booleanis appropriate because:
- The UI components in
CreateVariableSetDialog.tsxandOverviewContent.tsxexplicitly handle these three types with specific input components for each type:{typeof value.value === "string" && (...)} {typeof value.value === "number" && (...)} {typeof value.value === "boolean" && (...)}
The test file
job-variables-deployment.test.tsshows that target variables are used with primitive values (e.g.,"test1","test2", etc.) and there are no test cases involving complex objects or arrays.The codebase doesn't show any usage of complex data types (objects or arrays) for target variable values in any of the components or business logic.
The type restriction to primitive types is correct and aligns with the codebase usage
The
valuefield's type restriction tostring | number | booleanis appropriate as these are the only types being used throughout the application, and the UI is specifically built to handle just these primitive types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for assignments of complex data types to `value` in `target_variable`. # Test: Find occurrences where `value` is assigned an object or array. # Expected result: No matches, indicating `value` is only used with primitive types. rg --type js --pcre2 '(targetVariable\.value\s*=\s*\{)|(targetVariable\.value\s*=\s*\[)' -A 2Length of output: 95
Script:
#!/bin/bash # Let's try a more comprehensive search strategy: # 1. Search for any assignments or usage of target.value or targetVariable.value rg --type ts --type js "\.value\s*[=:]" -B 2 -A 2 # 2. Look for test files that might show example usages fd "test|spec" -e ts -e js --exec rg -l "target.*value" # 3. Search for type definitions or interfaces related to target value ast-grep --pattern 'interface $_ { $$$ value: $_ $$$ }'Length of output: 8337
Script:
#!/bin/bash # Let's check the test file found and look for actual usage examples cat ./packages/job-dispatch/src/__test__/job-variables-deployment.test.ts # Also search for any schema or type definitions specifically for target variables rg --type ts "type.*Target|interface.*Target" -B 2 -A 5 # And look for actual value assignments in the codebase rg --type ts "value:\s*(string|number|boolean|\{|\[)" -B 2 -A 2Length of output: 114177
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/EditTargetVariableDialog.tsx (1)
233-233: Verify the correct property for disabling the submit button.Please confirm that
updateTargetVariable.isPendingis the correct property to disable the submit button. Depending on the API, it might beisLoadingor another property.Run the following script to verify the available properties:
✅ Verification successful
Let me search for more context about the mutation usage to verify the correct property.
Let me search for the tRPC router definition to confirm the mutation type.
Let me try a different search pattern to find the tRPC router.
Let me try one last search without file type restriction.
Based on the search results, I can see that the code is using tRPC mutations with the
useMutationhook from a modern React query library. TheisPendingproperty is the correct property to use for disabling the submit button while the mutation is in progress, as it's the standard property provided by the library to indicate the loading state of a mutation.The usage of
updateTargetVariable.isPendingis correct for disabling the submit button.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check properties of the updateTargetVariable mutation. # Search for the mutation usage and list properties. rg 'const updateTargetVariable = api\.target\.variable\.update\.useMutation' -A 5Length of output: 878
Script:
#!/bin/bash # Search for the tRPC mutation definition and its type rg "variable\.update" -A 10Length of output: 3237
Script:
#!/bin/bash # Search for the tRPC router definition for target variable update ast-grep --pattern 'target: { $$$ variable: { $$$ update: $_ $$$ } $$$ }'Length of output: 95
Script:
#!/bin/bash # Search for the tRPC router definition for target variable update rg "target.*variable.*update" -A 5 --type typescriptLength of output: 89
Script:
#!/bin/bash # Search for any usage of the mutation to understand its properties rg "useMutation" -B 2 -A 2Length of output: 56293
packages/api/src/router/target.ts (1)
188-204: Logic for creating target variables looks goodThe implementation for creating target variables is correct, and the encryption of sensitive values is handled appropriately.
| @@ -23,6 +23,7 @@ | |||
| "@ctrlplane/db": "workspace:*", | |||
| "@ctrlplane/job-dispatch": "workspace:*", | |||
| "@ctrlplane/logger": "workspace:*", | |||
| "@ctrlplane/secrets": "workspace:*", | |||
There was a problem hiding this comment.
💡 Codebase verification
Secrets package implementation verified with some security concerns
The secrets package implements AES-256-CBC encryption with the following concerns:
- No test coverage found, which is critical for cryptographic implementations
- Missing security-related documentation and usage guidelines
- No input validation for the encryption/decryption methods
- No explicit error handling for cryptographic operations
- The IV is concatenated with the ciphertext using a simple delimiter (':') which could be problematic
Recommendations:
- Add comprehensive test suite covering edge cases and error scenarios
- Add input validation for the data parameter in encrypt/decrypt methods
- Implement proper error handling for cryptographic operations
- Consider using a more robust method to combine IV and ciphertext
- Add security documentation and usage guidelines
🔗 Analysis chain
Verify the secrets package implementation.
Since this package will be handling sensitive data for target variables, let's verify its implementation:
Let me gather more information about the encryption implementation and security practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the secrets package implementation and usage
# Test 1: Check for encryption-related code
echo "Checking encryption implementation..."
rg -g '!node_modules' -g '!dist' -A 5 'encrypt|decrypt' packages/secrets/
# Test 2: Check for security best practices in tests
echo "Checking test coverage..."
rg -g '!node_modules' -g '!dist' -l '\.test\.|\.spec\.' packages/secrets/
# Test 3: Check package exports
echo "Checking package exports..."
cat packages/secrets/package.json
Length of output: 2993
Script:
#!/bin/bash
# Check for the encryption algorithm and key generation
echo "Checking encryption configuration..."
rg -g '!node_modules' -g '!dist' -A 5 'algorithm.*=|key.*=' packages/secrets/
# Check for any security-related comments or documentation
echo "Checking security documentation..."
rg -g '!node_modules' -g '!dist' -B 2 -A 2 '@security|@encryption|security|encryption' packages/secrets/
# Check the complete implementation file
echo "Checking complete implementation..."
cat packages/secrets/src/index.ts
Length of output: 2382
| export const DeleteTargetVariableDialog: React.FC< | ||
| DeleteTargetVariableDialogProps | ||
| > = ({ variableId, targetId, onClose, children }) => { | ||
| const [open, setOpen] = useState(false); | ||
| const deleteTargetVariable = api.target.variable.delete.useMutation(); | ||
| const utils = api.useUtils(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding loading state management.
The component could benefit from handling loading states during the deletion process to prevent multiple clicks and provide better user feedback.
export const DeleteTargetVariableDialog: React.FC<
DeleteTargetVariableDialogProps
> = ({ variableId, targetId, onClose, children }) => {
const [open, setOpen] = useState(false);
+ const [isDeleting, setIsDeleting] = useState(false);
const deleteTargetVariable = api.target.variable.delete.useMutation();
const utils = api.useUtils();📝 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 DeleteTargetVariableDialog: React.FC< | |
| DeleteTargetVariableDialogProps | |
| > = ({ variableId, targetId, onClose, children }) => { | |
| const [open, setOpen] = useState(false); | |
| const deleteTargetVariable = api.target.variable.delete.useMutation(); | |
| const utils = api.useUtils(); | |
| export const DeleteTargetVariableDialog: React.FC< | |
| DeleteTargetVariableDialogProps | |
| > = ({ variableId, targetId, onClose, children }) => { | |
| const [open, setOpen] = useState(false); | |
| const [isDeleting, setIsDeleting] = useState(false); | |
| const deleteTargetVariable = api.target.variable.delete.useMutation(); | |
| const utils = api.useUtils(); | |
| return ( | ||
| <AlertDialog | ||
| open={open} | ||
| onOpenChange={(o) => { | ||
| setOpen(o); | ||
| if (!o) onClose(); | ||
| }} | ||
| > | ||
| <AlertDialogTrigger asChild>{children}</AlertDialogTrigger> | ||
| <AlertDialogContent> | ||
| <AlertDialogHeader>Are you sure?</AlertDialogHeader> | ||
| <AlertDialogDescription> | ||
| Deleting a target variable can change what values are passed to | ||
| pipelines running for this target. | ||
| </AlertDialogDescription> | ||
|
|
||
| <AlertDialogFooter> | ||
| <AlertDialogCancel>Cancel</AlertDialogCancel> | ||
| <div className="flex-grow" /> | ||
| <AlertDialogAction | ||
| className={buttonVariants({ variant: "destructive" })} | ||
| onClick={onDelete} | ||
| > | ||
| Delete | ||
| </AlertDialogAction> | ||
| </AlertDialogFooter> | ||
| </AlertDialogContent> | ||
| </AlertDialog> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance dialog UI to handle loading states.
The dialog UI should reflect the loading state during deletion and prevent user interactions while processing.
<AlertDialogFooter>
- <AlertDialogCancel>Cancel</AlertDialogCancel>
+ <AlertDialogCancel disabled={isDeleting}>Cancel</AlertDialogCancel>
<div className="flex-grow" />
<AlertDialogAction
className={buttonVariants({ variant: "destructive" })}
onClick={onDelete}
+ disabled={isDeleting}
>
- Delete
+ {isDeleting ? "Deleting..." : "Delete"}
</AlertDialogAction>
</AlertDialogFooter>Committable suggestion was skipped due to low confidence.
| const onDelete = () => | ||
| deleteTargetVariable | ||
| .mutateAsync(variableId) | ||
| .then(() => utils.target.byId.invalidate(targetId)) | ||
| .then(() => setOpen(false)); |
There was a problem hiding this comment.
Enhance error handling and user feedback in delete operation.
The current implementation lacks error handling and user feedback. Consider implementing a more robust deletion flow.
- const onDelete = () =>
- deleteTargetVariable
- .mutateAsync(variableId)
- .then(() => utils.target.byId.invalidate(targetId))
- .then(() => setOpen(false));
+ const onDelete = async () => {
+ try {
+ setIsDeleting(true);
+ await deleteTargetVariable.mutateAsync(variableId);
+ await utils.target.byId.invalidate(targetId);
+ setOpen(false);
+ // Consider adding a toast notification for success
+ } catch (error) {
+ // Consider adding error handling UI (e.g., toast notification)
+ console.error('Failed to delete target variable:', error);
+ } finally {
+ setIsDeleting(false);
+ }
+ };Committable suggestion was skipped due to low confidence.
| return ( | ||
| <DropdownMenu open={open} onOpenChange={setOpen}> | ||
| <DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger> | ||
| <DropdownMenuContent align="end"> | ||
| <EditTargetVariableDialog | ||
| targetVariable={targetVariable} | ||
| existingKeys={existingKeys} | ||
| onClose={() => setOpen(false)} | ||
| > | ||
| <DropdownMenuItem | ||
| className="flex cursor-pointer items-center gap-2" | ||
| onSelect={(e) => e.preventDefault()} | ||
| > | ||
| <IconPencil className="h-4 w-4" /> | ||
| Edit | ||
| </DropdownMenuItem> | ||
| </EditTargetVariableDialog> | ||
| <DeleteTargetVariableDialog | ||
| variableId={targetVariable.id} | ||
| targetId={targetVariable.targetId} | ||
| onClose={() => setOpen(false)} | ||
| > | ||
| <DropdownMenuItem | ||
| className="flex cursor-pointer items-center gap-2" | ||
| onSelect={(e) => e.preventDefault()} | ||
| > | ||
| <IconTrash className="h-4 w-4 text-red-500" /> | ||
| Delete | ||
| </DropdownMenuItem> | ||
| </DeleteTargetVariableDialog> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| ); |
There was a problem hiding this comment.
Add accessibility attributes and improve event handling.
The dropdown implementation could be enhanced with the following improvements:
Apply these changes to improve accessibility and event handling:
- <DropdownMenu open={open} onOpenChange={setOpen}>
+ <DropdownMenu
+ open={open}
+ onOpenChange={setOpen}
+ aria-label="Target variable options"
+ >
<DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger>
<DropdownMenuContent align="end">
<EditTargetVariableDialog
targetVariable={targetVariable}
existingKeys={existingKeys}
- onClose={() => setOpen(false)}
+ onClose={(success?: boolean) => {
+ setOpen(false);
+ // Handle success/error state if needed
+ }}
>
<DropdownMenuItem
className="flex cursor-pointer items-center gap-2"
- onSelect={(e) => e.preventDefault()}
+ onSelect={(e) => {
+ e.preventDefault();
+ e.stopPropagation();
+ }}Committable suggestion was skipped due to low confidence.
| value={value as number} | ||
| onChange={onChange} | ||
| type="number" | ||
| /> |
There was a problem hiding this comment.
Fix number input to correctly handle numeric values.
The onChange handler for the number input may not correctly convert the input value to a number, potentially causing validation issues since form values might be treated as strings.
Apply this diff to parse the input value to a number:
<Input
value={value as number}
- onChange={onChange}
+ onChange={(e) => onChange(Number(e.target.value))}
type="number"
/>📝 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.
| value={value as number} | |
| onChange={onChange} | |
| type="number" | |
| /> | |
| value={value as number} | |
| onChange={(e) => onChange(Number(e.target.value))} | |
| type="number" | |
| /> |
| const onSubmit = form.handleSubmit((data) => | ||
| updateTargetVariable | ||
| .mutateAsync({ id: targetVariable.id, data }) | ||
| .then(() => form.reset(data)) | ||
| .then(() => utils.target.byId.invalidate(targetVariable.targetId)) | ||
| .then(() => setOpen(false)), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the form submission process.
Errors during the mutation are not currently handled. Adding error handling will improve user experience by providing feedback when an update fails.
Apply this diff to add error handling:
const onSubmit = form.handleSubmit((data) =>
updateTargetVariable
.mutateAsync({ id: targetVariable.id, data })
.then(() => form.reset(data))
.then(() => utils.target.byId.invalidate(targetVariable.targetId))
.then(() => setOpen(false))
+ .catch((error) => {
+ // Handle error, e.g., display an error message
+ console.error(error);
+ form.setError("root", { message: "Failed to update target variable." });
+ }),
);📝 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 onSubmit = form.handleSubmit((data) => | |
| updateTargetVariable | |
| .mutateAsync({ id: targetVariable.id, data }) | |
| .then(() => form.reset(data)) | |
| .then(() => utils.target.byId.invalidate(targetVariable.targetId)) | |
| .then(() => setOpen(false)), | |
| ); | |
| const onSubmit = form.handleSubmit((data) => | |
| updateTargetVariable | |
| .mutateAsync({ id: targetVariable.id, data }) | |
| .then(() => form.reset(data)) | |
| .then(() => utils.target.byId.invalidate(targetVariable.targetId)) | |
| .then(() => setOpen(false)) | |
| .catch((error) => { | |
| // Handle error, e.g., display an error message | |
| console.error(error); | |
| form.setError("root", { message: "Failed to update target variable." }); | |
| }), | |
| ); |
| render={({ field: { value, onChange } }) => { | ||
| const onSensitiveChange = (checked: boolean) => { | ||
| if (!checked) form.setValue("value", ""); | ||
| onChange(checked); | ||
| }; |
There was a problem hiding this comment.
Review the logic for clearing the value when 'Sensitive' is toggled.
Currently, when the Sensitive checkbox is unchecked, the value is cleared. If the intention is to clear the value when the checkbox is checked (marking it as sensitive), you should reverse the condition in onSensitiveChange.
Apply this diff to correct the logic:
const onSensitiveChange = (checked: boolean) => {
- if (!checked) form.setValue("value", "");
+ if (checked) form.setValue("value", "");
onChange(checked);
};Committable suggestion was skipped due to low confidence.
| const { sensitive } = input.data; | ||
| const value = sensitive | ||
| ? variablesAES256().encrypt(String(input.data.value)) | ||
| : input.data.value; | ||
| const data = { ...input.data, value }; |
There was a problem hiding this comment.
Ensure 'value' is defined when updating variables
When updating a target variable, if input.data.value is undefined, converting it with String(input.data.value) will result in the string "undefined", which may not be the intended behavior. Consider adding validation to ensure that input.data.value is defined when updating the variable's value, especially when sensitive is true.
Apply this diff to add a check for input.data.value:
const { sensitive, value: inputValue } = input.data;
+ if (inputValue === undefined) {
+ throw new Error('Variable value must be provided when updating.');
+ }
const value = sensitive
? variablesAES256().encrypt(String(inputValue))
: inputValue;📝 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 { sensitive } = input.data; | |
| const value = sensitive | |
| ? variablesAES256().encrypt(String(input.data.value)) | |
| : input.data.value; | |
| const data = { ...input.data, value }; | |
| const { sensitive, value: inputValue } = input.data; | |
| if (inputValue === undefined) { | |
| throw new Error('Variable value must be provided when updating.'); | |
| } | |
| const value = sensitive | |
| ? variablesAES256().encrypt(String(inputValue)) | |
| : inputValue; | |
| const data = { ...input.data, value }; |
| .query(({ ctx, input }) => | ||
| ctx.db.query.target | ||
| .findFirst({ | ||
| where: eq(schema.target.id, input), | ||
| with: { metadata: true, variables: true, provider: true }, | ||
| }) | ||
| .then((t) => { | ||
| if (t == null) return null; | ||
| const pairs = t.metadata.map((m) => [m.key, m.value]); | ||
| const metadata = Object.fromEntries(pairs); | ||
| return { ...t, metadata }; | ||
| }), | ||
| ), |
There was a problem hiding this comment.
Security Issue: Sensitive variable values may be exposed in API response
In the byId query, the variables associated with the target are retrieved and returned directly. If any of these variables are sensitive, their values might be exposed to users who call this API endpoint. To prevent leaking sensitive information, consider masking sensitive variable values or excluding them from the response.
Apply this diff to mask sensitive variable values in the response:
.then((t) => {
if (t == null) return null;
const pairs = t.metadata.map((m) => [m.key, m.value]);
const metadata = Object.fromEntries(pairs);
+ const variables = t.variables.map((v) => ({
+ ...v,
+ value: v.sensitive ? null : v.value,
+ }));
- return { ...t, metadata };
+ return { ...t, metadata, variables };
}),📝 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.
| .query(({ ctx, input }) => | |
| ctx.db.query.target | |
| .findFirst({ | |
| where: eq(schema.target.id, input), | |
| with: { metadata: true, variables: true, provider: true }, | |
| }) | |
| .then((t) => { | |
| if (t == null) return null; | |
| const pairs = t.metadata.map((m) => [m.key, m.value]); | |
| const metadata = Object.fromEntries(pairs); | |
| return { ...t, metadata }; | |
| }), | |
| ), | |
| .query(({ ctx, input }) => | |
| ctx.db.query.target | |
| .findFirst({ | |
| where: eq(schema.target.id, input), | |
| with: { metadata: true, variables: true, provider: true }, | |
| }) | |
| .then((t) => { | |
| if (t == null) return null; | |
| const pairs = t.metadata.map((m) => [m.key, m.value]); | |
| const metadata = Object.fromEntries(pairs); | |
| const variables = t.variables.map((v) => ({ | |
| ...v, | |
| value: v.sensitive ? null : v.value, | |
| })); | |
| return { ...t, metadata, variables }; | |
| }), | |
| ), |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/DeleteTargetVariableDialog.tsx (2)
48-51: Enhance the warning message with more specific details.The current warning message is generic. Consider making it more specific by including the variable name and potential impact.
- Deleting a target variable can change what values are passed to - pipelines running for this target. + Deleting this target variable will permanently remove it and may affect + pipeline executions that depend on this variable. This action cannot be + undone.
56-62: Improve button text during deletion.The delete button should indicate when the deletion is in progress.
<AlertDialogAction className={buttonVariants({ variant: "destructive" })} onClick={onDelete} disabled={deleteTargetVariable.isPending} > - Delete + {deleteTargetVariable.isPending ? "Deleting..." : "Delete"} </AlertDialogAction>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/DeleteTargetVariableDialog.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx
🔇 Additional comments (4)
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/DeleteTargetVariableDialog.tsx (4)
1-22: LGTM! Well-structured imports and type definitions.The imports are properly organized and the type definition is clear with all necessary props.
24-30: Loading state management comment is still applicable.Referring to the previous review comment about adding loading state management.
31-35: Error handling comment is still applicable.Referring to the previous review comment about enhancing error handling and user feedback.
28-29: Verify the target variable deletion API endpoint.Let's verify that the API endpoint for target variable deletion is properly implemented.
| value: z.union([z.string(), z.number(), z.boolean()]).refine((v) => { | ||
| if (typeof v === "string") return v.length > 0; | ||
| return true; | ||
| }), |
There was a problem hiding this comment.
| value: z.union([z.string(), z.number(), z.boolean()]).refine((v) => { | |
| if (typeof v === "string") return v.length > 0; | |
| return true; | |
| }), | |
| value: z.union([z.string(), z.number(), z.boolean()]).refine((v) => | |
| typeof v === "string" ? v.length > 0 : true | |
| ), |
Summary by CodeRabbit
Release Notes
New Features
CreateTargetVariableDialog,EditTargetVariableDialog, andDeleteTargetVariableDialogfor managing target variables through a user-friendly dialog interface.TargetVariableDropdownfor streamlined editing and deletion of target variables.TargetVariableSectionto display target variables in a structured format, enhancing user experience.Improvements
VariableContentcomponent to accept and display target variables.Dependencies
@ctrlplane/secretsas a new dependency for improved functionality.