fix: Variable ui cleanup #203
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the 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: 2
🧹 Outside diff range and nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/runbooks/RunbookRow.tsx (1)
46-53: Consider removing redundant cursor-pointer classThe
cursor-pointerclass might be redundant asDropdownMenuItemcomponents typically handle cursor styling internally.- className="flex cursor-pointer items-center gap-2" + className="flex items-center gap-2"apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/ConfigFields.tsx (2)
44-55: Consider type safety improvements and performance optimizationWhile the implementation works, there are a few areas for improvement:
- Type Safety: Consider using a union type for excluded types to prevent invalid strings:
type VariableType = "string" | "number" | "boolean" | "choice"; exclude?: VariableType[];
- Performance: For larger exclude lists, consider using a Set for O(1) lookups:
const excludeSet = new Set(exclude); // Then use: !excludeSet.has("string")
44-55: Consider maintaining consistency with RunbookConfigTypeSelectorThe
RunbookConfigTypeSelectorcomponent has similar functionality but doesn't support theexcludeprop. Consider either:
- Adding the
excludeprop toRunbookConfigTypeSelectorfor consistency- Creating a shared base component that both selectors extend
This would improve maintainability and reduce code duplication.
packages/api/src/router/runbook.ts (1)
109-129: Refactor transaction logic using async/await for better readability and consistencyConsider refactoring the transaction logic to use
async/awaitinstead of chained.then()calls. This will improve readability and make error handling more straightforward. Additionally, returning the updated runbook along with its variables, similar to thecreatemutation, would maintain consistency in API responses.Apply this diff to refactor the transaction logic:
- ctx.db.transaction((tx) => - tx - .update(SCHEMA.runbook) - .set(input.data) - .where(eq(SCHEMA.runbook.id, input.id)) - .returning() - .then(takeFirst) - .then((rb) => - tx - .delete(SCHEMA.runbookVariable) - .where(eq(SCHEMA.runbookVariable.runbookId, rb.id)) - .then(() => - tx.insert(SCHEMA.runbookVariable).values( - input.data.variables.map((v) => ({ - ...v, - runbookId: rb.id, - })), - ), - ), - ), - ), + ctx.db.transaction(async (tx) => { + const rb = await tx + .update(SCHEMA.runbook) + .set(input.data) + .where(eq(SCHEMA.runbook.id, input.id)) + .returning() + .then(takeFirst); + + await tx + .delete(SCHEMA.runbookVariable) + .where(eq(SCHEMA.runbookVariable.runbookId, rb.id)); + + const vars = await tx.insert(SCHEMA.runbookVariable).values( + input.data.variables.map((v) => ({ + ...v, + runbookId: rb.id, + })), + ).returning(); + + return { ...rb, variables: vars }; + }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/ConfigFields.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/CreateVariableDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/runbooks/EditRunbookDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/runbooks/RunbookRow.tsx(3 hunks)packages/api/src/router/runbook.ts(1 hunks)
🔇 Additional comments (8)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/runbooks/RunbookRow.tsx (3)
5-5: LGTM! Import changes are well-structured
The new imports are properly scoped and follow good practices.
Also applies to: 15-15
54-66: Consider adding error boundary and removing redundant class
- The cursor-pointer class issue mentioned above applies here as well.
- Consider wrapping the EditRunbookDialog in an error boundary to gracefully handle any rendering errors.
import { ErrorBoundary } from 'react-error-boundary';
// Wrap the dialog in an error boundary
<ErrorBoundary fallback={<div>Something went wrong with the runbook editor</div>}>
<EditRunbookDialog
runbook={runbook}
workspace={workspace}
jobAgents={jobAgents}
>
{/* ... */}
</EditRunbookDialog>
</ErrorBoundary>54-58: Verify EditRunbookDialog implementation
Let's ensure the new EditRunbookDialog component is properly implemented with the expected props interface.
✅ Verification successful
EditRunbookDialog integration is correctly implemented
The component exists and its props interface matches the usage perfectly:
- Component accepts required props:
workspace,jobAgents,runbook(with nested variables) - Props types are properly defined and match the schema types from
@ctrlplane/db/schema - The component is correctly used in RunbookRow with all required props being passed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify EditRunbookDialog implementation and props interface
# Check if the component file exists and contains the expected props
rg -A 10 "export.*EditRunbookDialog.*=.*{" --type typescript
# Check for proper prop types definition
rg "interface.*EditRunbookDialog.*Props" --type typescript
Length of output: 284
Script:
#!/bin/bash
# Search for EditRunbookDialog component definition
rg -l "EditRunbookDialog"
# Find the component implementation
ast-grep --pattern 'export const EditRunbookDialog = $_'
# Find interface/type definitions for props
ast-grep --pattern 'interface EditRunbookDialogProps {
$$$
}'
# Find type definitions using type keyword
ast-grep --pattern 'type EditRunbookDialogProps = {
$$$
}'
Length of output: 390
Script:
#!/bin/bash
# Let's examine the EditRunbookDialog component implementation
cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/runbooks/EditRunbookDialog.tsx"
# Also check the actual usage in RunbookRow
cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/runbooks/RunbookRow.tsx"
Length of output: 8742
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/CreateVariableDialog.tsx (2)
117-118: LGTM! Clean and focused changes.
The changes effectively remove the "choice" type configuration by:
- Simplifying the onChange handler to directly update the type
- Explicitly excluding the "choice" type using the new
excludeprop
This aligns well with the PR objective of cleaning up the variable UI.
117-118: Verify impact on existing variables.
Since we're removing support for "choice" type variables, we should verify that there are no existing variables of this type that would be affected.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/ConfigFields.tsx (2)
37-38: LGTM: Clean prop addition
The new optional exclude prop is well-typed and follows TypeScript best practices.
37-55: Verify usage of ConfigTypeSelector
Let's verify that all existing usages of ConfigTypeSelector handle the new optional prop correctly.
✅ Verification successful
The new exclude prop is safely implemented and used correctly
The verification shows that:
- There are only two usages of
ConfigTypeSelector:- In
CreateVariableDialog.tsxwhere it's used withexclude={["choice"]}, properly handling the new prop - A separate
RunbookConfigTypeSelectorcomponent that doesn't use the exclude prop, as it's a different component
- In
Since the exclude prop is optional and the only existing usage already provides the prop correctly, there are no breaking changes or issues to address.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of ConfigTypeSelector
ast-grep --pattern 'ConfigTypeSelector $$$'
# Also check for any potential test files
fd -e test.tsx -e spec.tsx -x grep -l "ConfigTypeSelector"
Length of output: 104
Script:
#!/bin/bash
# Search for usages of ConfigTypeSelector with ripgrep for better context
rg "ConfigTypeSelector" -A 3 -B 3
# Also search for any imports of ConfigTypeSelector
rg "import.*ConfigTypeSelector"
Length of output: 6121
packages/api/src/router/runbook.ts (1)
103-105: The addition of 'variables' to the update input schema is appropriate
Extending data to include variables in the update mutation aligns with the handling of runbook variables and ensures they can be updated correctly.
| defaultValues: { | ||
| ...runbook, | ||
| description: runbook.description ?? "", | ||
| jobAgentId: runbook.jobAgentId ?? "", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid spreading runbook into defaultValues
Spreading ...runbook into defaultValues may include unexpected fields that are not defined in the form schema, potentially causing validation issues or unintended behavior. It's better to explicitly set only the fields defined in the schema.
Apply this diff to only include necessary fields:
defaultValues: {
- ...runbook,
- description: runbook.description ?? "",
- jobAgentId: runbook.jobAgentId ?? "",
+ name: runbook.name,
+ description: runbook.description ?? "",
+ variables: runbook.variables,
+ jobAgentId: runbook.jobAgentId ?? "",
+ jobAgentConfig: runbook.jobAgentConfig ?? {},
},📝 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.
| defaultValues: { | |
| ...runbook, | |
| description: runbook.description ?? "", | |
| jobAgentId: runbook.jobAgentId ?? "", | |
| }, | |
| defaultValues: { | |
| name: runbook.name, | |
| description: runbook.description ?? "", | |
| variables: runbook.variables, | |
| jobAgentId: runbook.jobAgentId ?? "", | |
| jobAgentConfig: runbook.jobAgentConfig ?? {}, | |
| }, |
| update | ||
| .mutateAsync({ id: runbook.id, data }) | ||
| .then(() => router.refresh()) | ||
| .then(() => setOpen(false)), | ||
| ); |
There was a problem hiding this comment.
Handle errors in form submission
The onSubmit handler does not handle errors from the mutation or router refresh. This could lead to unhandled promise rejections and a poor user experience if the update fails.
Apply this diff to handle errors using try...catch:
-const onSubmit = form.handleSubmit(async (data) =>
- update
- .mutateAsync({ id: runbook.id, data })
- .then(() => router.refresh())
- .then(() => setOpen(false)),
-);
+const onSubmit = form.handleSubmit(async (data) => {
+ try {
+ await update.mutateAsync({ id: runbook.id, data });
+ router.refresh();
+ setOpen(false);
+ } catch (error) {
+ // Handle error, e.g., display error message to the user
+ form.setError("root", { message: "Failed to update runbook." });
+ }
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| update | |
| .mutateAsync({ id: runbook.id, data }) | |
| .then(() => router.refresh()) | |
| .then(() => setOpen(false)), | |
| ); | |
| const onSubmit = form.handleSubmit(async (data) => { | |
| try { | |
| await update.mutateAsync({ id: runbook.id, data }); | |
| router.refresh(); | |
| setOpen(false); | |
| } catch (error) { | |
| // Handle error, e.g., display error message to the user | |
| form.setError("root", { message: "Failed to update runbook." }); | |
| } | |
| }); |
Summary by CodeRabbit
New Features
EditRunbookDialogfor editing runbooks with enhanced form validation and state management.excludeprop to theConfigTypeSelectorto hide specific variable types.Bug Fixes
Improvements
RunbookRowfor a more streamlined management experience of runbooks.