fix: Variable sets #150
fix: Variable sets #150
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes include the introduction of new React components and hooks for managing variable sets within a user interface, along with enhancements to existing components and APIs. A new SQL table for variable set assignments is created, and several database schema modifications are made to improve relational capabilities. Additionally, the router methods for variable sets are updated to utilize a structured query builder, enhancing data retrieval and manipulation. Tests are added for job variable management, and utility functions are introduced to streamline database interactions related to job dispatching. 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
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: 43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
- apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/OverviewContent.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/VariableSetDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/layout.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/CreateValueSetDialog.tsx (0 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/CreateVariableSetDialog.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/GettingStartedVariableSets.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/VariableSetsTable.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/page.tsx (2 hunks)
- packages/api/src/router/system.ts (1 hunks)
- packages/api/src/router/variable-set.ts (4 hunks)
- packages/db/drizzle/0016_cute_vengeance.sql (1 hunks)
- packages/db/drizzle/meta/_journal.json (1 hunks)
- packages/db/src/schema/environment.ts (3 hunks)
- packages/db/src/schema/variable-sets.ts (3 hunks)
- packages/job-dispatch/package.json (2 hunks)
- packages/job-dispatch/src/test/job-variables-deployment.test.ts (1 hunks)
- packages/job-dispatch/src/job-variables-deployment.ts (0 hunks)
- packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts (1 hunks)
- packages/job-dispatch/src/job-variables-deployment/utils.ts (1 hunks)
- packages/job-dispatch/src/release-job-trigger.ts (1 hunks)
💤 Files with no reviewable changes (2)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/CreateValueSetDialog.tsx
- packages/job-dispatch/src/job-variables-deployment.ts
🧰 Additional context used
🪛 Biome
apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/OverviewContent.tsx
[error] 254-254: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/CreateVariableSetDialog.tsx
[error] 270-270: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/api/src/router/system.ts
[error] 135-135: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/job-dispatch/src/__test__/job-variables-deployment.test.ts
[error] 125-125: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 138-138: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 138-138: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 173-173: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 173-173: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 208-208: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 208-208: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 232-232: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 275-275: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 275-275: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 276-276: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 300-300: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 343-343: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 343-343: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 408-408: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 408-408: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 473-473: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 557-557: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts
[error] 99-99: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 146-146: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (25)
packages/db/drizzle/0016_cute_vengeance.sql (4)
1-5: LGTM: Well-structured table creationThe "variable_set_assignment" table is well-designed with appropriate column types and constraints. Using UUIDs for all columns and setting up a primary key with a default value is a good practice.
7-9: Verify data migration for "value" column changeThe modification of the "value" column in the "variable_set_value" table from its previous type to jsonb is a significant change. Please ensure that:
- Any existing data has been migrated to the new jsonb format.
- The application code handling this data has been updated accordingly.
- The NOT NULL constraint won't cause issues with existing rows.
Could you provide information on how the data migration is being handled for this change?
11-15: LGTM: Appropriate foreign key constraintThe foreign key constraint linking "variable_set_assignment" to "variable_set" is well-defined. The CASCADE delete behavior ensures referential integrity, and the error handling for potential duplicate constraints is a good practice.
17-21: LGTM: Well-defined foreign key constraintThe foreign key constraint linking "variable_set_assignment" to "environment" is correctly implemented. The CASCADE delete behavior maintains referential integrity, and the error handling for potential duplicate constraints is appropriate.
apps/webservice/src/app/[workspaceSlug]/layout.tsx (3)
10-10: LGTM: Import statement for VariableSetDrawerThe import statement for
VariableSetDraweris correctly added and follows the established pattern for other drawer components in this file. This addition aligns with the PR objective of addressing variable sets.
Line range hint
1-43: Summary: Minimal, well-integrated changes to WorkspaceLayoutThe changes to the
WorkspaceLayoutcomponent are minimal and well-integrated. The addition of theVariableSetDrawercomponent enhances the workspace functionality without disrupting existing features. The changes follow the established patterns in the file and don't introduce any apparent performance issues.
41-41: LGTM: VariableSetDrawer component addedThe
VariableSetDrawercomponent is correctly added to the JSX, consistent with the placement of other drawer components. This addition enhances the UI with new functionality for managing variable sets, aligning with the PR objectives.To ensure the component is properly implemented, please verify:
- The
VariableSetDrawercomponent exists and is correctly exported from the specified path.- The component doesn't require any props, similar to other drawer components.
You can run the following script to verify the component's existence and export:
packages/job-dispatch/package.json (2)
25-25: LGTM: Test script addedThe addition of the "test" script using Vitest is a good practice. It provides a standardized way to run tests for this package.
54-54: LGTM: Vitest version updatedUpdating the Vitest version from ^2.1.1 to ^2.1.2 is a good practice. This patch update likely includes bug fixes and minor improvements.
To ensure compatibility with other dependencies, let's check for any potential conflicts:
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/GettingStartedVariableSets.tsx (3)
1-1: LGTM: Necessary import added for type definition.The import of the SCHEMA type is correctly added to support the new prop type definition for environments.
6-6: Good catch: Import statement corrected.The import statement has been correctly updated from "CreateValueSetDialog" to "CreateVariableSetDialog". This change is crucial for maintaining consistency and preventing potential runtime errors.
31-34: CreateVariableSetDialog usage updated correctly. Verify dialog implementation.The
CreateVariableSetDialogcomponent is now correctly receiving bothsystemIdandenvironmentsprops, which is consistent with the changes made to the parent component.To ensure the changes are fully implemented, please verify that the
CreateVariableSetDialogcomponent is updated to handle the newenvironmentsprop. Run the following script to check its implementation:If the script doesn't return any results, it might indicate that the
CreateVariableSetDialogcomponent needs to be updated to properly utilize the newenvironmentsprop.apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/VariableSetsTable.tsx (1)
1-15: LGTM: Imports and client-side rendering directive are properly set up.The "use client" directive is correctly placed, and the necessary imports are well-organized. The use of a custom hook (
useVariableSetDrawer) suggests good separation of concerns.packages/db/drizzle/meta/_journal.json (1)
117-122: LGTM! New migration entry correctly added.The new entry for migration "0016_cute_vengeance" has been correctly added to the journal:
- It follows the existing structure and pattern of previous entries.
- The index (idx) is correctly incremented to 16.
- The timestamp (when) is newer than the previous entry, maintaining chronological order.
- All required properties are present and consistent with the file's format.
This addition maintains the integrity and consistency of the migration journal.
packages/db/src/schema/environment.ts (3)
4-4: LGTM: Import statement updated correctly.The addition of
relationsto the import from 'drizzle-orm' is appropriate for the new relational definition introduced later in the file.
24-24: LGTM: New import added correctly.The import of
variableSetAssignmentis consistent with the file's import style and is necessary for the new relation definition.
Line range hint
1-54: Summary: Enhancements to environment schema relationsThe changes in this file successfully introduce a new relation between environments and variable set assignments. This enhancement aligns with the PR objective of fixing variable sets and improves the relational capabilities of the schema. The modifications are well-structured and consistent with the existing code style.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/page.tsx (1)
9-11: Approved: Correctly Updated ImportsThe imports for
CreateVariableSetDialogandVariableSetsTablehave been added appropriately, aligning with the components used later in the code.packages/job-dispatch/src/job-variables-deployment/utils.ts (3)
50-64: Verify the correctness ofgetMatchedTargetfunction parameters.Ensure that the
SCHEMA.targetMatchesMetadatafunction accepts thetxandtargetFilterparameters as used. Confirm that the function returns a condition suitable for thewhereclause.Run the following script to check the definition of
targetMatchesMetadata:#!/bin/bash # Description: Verify the signature of targetMatchesMetadata. # Test: Search for the function definition. Expect: Function accepts (tx, targetFilter) and returns a condition. ast-grep --pattern $'function targetMatchesMetadata(tx, targetFilter) { $$$ }'
28-34: Handle potentialnullreturns ingetTarget.The
getTargetfunction uses.then(takeFirstOrNull);, which may returnnullif no matching target is found. Ensure that calling functions handlenullappropriately to avoid runtime errors.Check where
getTargetis used and confirm thatnullcases are handled.#!/bin/bash # Description: Find usages of getTarget and verify null handling. # Test: Search for usages of getTarget. Expect: Null checks are implemented. rg -A 3 $'getTarget' --glob '!packages/job-dispatch/src/job-variables-deployment/utils.ts'
18-26: Ensure correct inner join and data retrieval ingetDeploymentVariables.The
getDeploymentVariablesfunction performs aninnerJoinbetweendeploymentVariableandreleaseusingdeploymentId. Verify that all relevantdeploymentVariableentries have correspondingreleaserecords. If there aredeploymentVariablerecords without a matchingrelease, they will be excluded. Consider whether aleftJoinmight be more appropriate to include all deployment variables.Run the following script to check for
deploymentVariableentries without matchingreleaserecords:packages/db/src/schema/variable-sets.ts (3)
32-41: Ensure optional fields are properly defined inupdateVariableSet.While extending
createVariableSet.partial(), make sure that optional fields are correctly specified, and validation remains consistent.
88-100: Ensure consistency in defining relations forvariableSetAssignmentRelations.The relationships are correctly established. Just make sure that similar patterns are followed across all relation definitions for maintainability.
74-82: 🧹 Nitpick (assertive)Review
onDelete: "cascade"behavior in foreign key constraints.In
variableSetAssignment, cascading deletes onvariableSetIdandenvironmentIdmean deleting a variable set or environment will remove related assignments. Confirm this is intended to prevent unexpected data loss.Check your application's deletion flows to ensure cascading deletes align with business logic.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/CreateVariableSetDialog.tsx (1)
57-72: Validation schema is well-defined.The Zod schema accurately enforces validation rules for the form inputs, ensuring data integrity and preventing invalid data submission.
| export const VariableSetGettingStarted: React.FC<{ | ||
| systemId: string; | ||
| environments: SCHEMA.Environment[]; | ||
| }> = ({ systemId, environments }) => { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Props update looks good. Consider type alias for improved readability.
The component props have been correctly updated to include the environments prop with the proper type annotation. This change aligns with the new requirements for the component.
For improved readability, consider creating a type alias for the props:
type VariableSetGettingStartedProps = {
systemId: string;
environments: SCHEMA.Environment[];
};
export const VariableSetGettingStarted: React.FC<VariableSetGettingStartedProps> = ({ systemId, environments }) => {
// ...
};This approach can make the component definition cleaner, especially if more props are added in the future.
| export const VariableSetsTable: React.FC<{ | ||
| variableSets: (SCHEMA.VariableSet & { | ||
| values: SCHEMA.VariableSetValue[]; | ||
| assignments: (SCHEMA.VariableSetAssignment & { | ||
| environment: SCHEMA.Environment; | ||
| })[]; | ||
| })[]; | ||
| }> = ({ variableSets }) => { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Well-defined component interface with proper typing.
The component's prop interface is clearly defined using TypeScript, which enhances type safety. The use of SCHEMA types ensures consistency with the database schema.
Consider normalizing the data structure or implementing pagination for better performance with large datasets. The current nested structure might lead to inefficient rendering or unnecessary re-renders if the dataset grows significantly.
| })[]; | ||
| })[]; | ||
| }> = ({ variableSets }) => { | ||
| const { setVariableSetId } = useVariableSetDrawer(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Good use of custom hook and event handling.
The component effectively uses the useVariableSetDrawer hook to manage the variable set drawer state. The click handler on the table row provides a good user interaction for opening the drawer.
Consider enhancing accessibility by adding keyboard navigation support. You could add a tabIndex to the row and handle the onKeyDown event to allow users to open the drawer using the keyboard:
<TableRow
key={variableSet.id}
onClick={() => setVariableSetId(variableSet.id)}
+ onKeyDown={(e) => e.key === 'Enter' && setVariableSetId(variableSet.id)}
+ tabIndex={0}
className="cursor-pointer"
>Also applies to: 38-42
| return ( | ||
| <Table> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead>Name</TableHead> | ||
| <TableHead>Values</TableHead> | ||
| <TableHead>Environments</TableHead> | ||
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody> | ||
| {variableSets.map((variableSet) => ( | ||
| <TableRow | ||
| key={variableSet.id} | ||
| onClick={() => setVariableSetId(variableSet.id)} | ||
| className="cursor-pointer" | ||
| > | ||
| <TableCell>{variableSet.name}</TableCell> | ||
| <TableCell>{variableSet.values.length}</TableCell> | ||
| <TableCell> | ||
| <div className="flex gap-1 overflow-x-auto"> | ||
| {variableSet.assignments.map((assignment) => ( | ||
| <Badge | ||
| key={assignment.id} | ||
| className="flex-shrink-0" | ||
| variant="secondary" | ||
| > | ||
| {assignment.environment.name} | ||
| </Badge> | ||
| ))} | ||
| </div> | ||
| </TableCell> | ||
| </TableRow> | ||
| ))} | ||
| </TableBody> | ||
| </Table> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Well-structured table with efficient rendering.
The table structure is clear and well-organized. The use of mapping to render rows is efficient, and the display of environment badges in a flex container with overflow handling is a good approach for varying numbers of environments.
Consider adding a visual indicator or tooltip for the number of hidden environments when there are too many to display in the available space. This could improve the user experience by providing more information at a glance:
<TableCell>
<div className="flex gap-1 overflow-x-auto">
{variableSet.assignments.slice(0, 3).map((assignment) => (
<Badge
key={assignment.id}
className="flex-shrink-0"
variant="secondary"
>
{assignment.environment.name}
</Badge>
))}
{variableSet.assignments.length > 3 && (
<Badge
className="flex-shrink-0"
variant="secondary"
title={`${variableSet.assignments.length - 3} more environments`}
>
+{variableSet.assignments.length - 3}
</Badge>
)}
</div>
</TableCell>This change would display up to 3 environment badges and then show a count of additional environments if there are more.
| export const VariableSetsTable: React.FC<{ | ||
| variableSets: (SCHEMA.VariableSet & { | ||
| values: SCHEMA.VariableSetValue[]; | ||
| assignments: (SCHEMA.VariableSetAssignment & { | ||
| environment: SCHEMA.Environment; | ||
| })[]; | ||
| })[]; | ||
| }> = ({ variableSets }) => { | ||
| const { setVariableSetId } = useVariableSetDrawer(); | ||
|
|
||
| return ( | ||
| <Table> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead>Name</TableHead> | ||
| <TableHead>Values</TableHead> | ||
| <TableHead>Environments</TableHead> | ||
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody> | ||
| {variableSets.map((variableSet) => ( | ||
| <TableRow | ||
| key={variableSet.id} | ||
| onClick={() => setVariableSetId(variableSet.id)} | ||
| className="cursor-pointer" | ||
| > | ||
| <TableCell>{variableSet.name}</TableCell> | ||
| <TableCell>{variableSet.values.length}</TableCell> | ||
| <TableCell> | ||
| <div className="flex gap-1 overflow-x-auto"> | ||
| {variableSet.assignments.map((assignment) => ( | ||
| <Badge | ||
| key={assignment.id} | ||
| className="flex-shrink-0" | ||
| variant="secondary" | ||
| > | ||
| {assignment.environment.name} | ||
| </Badge> | ||
| ))} | ||
| </div> | ||
| </TableCell> | ||
| </TableRow> | ||
| ))} | ||
| </TableBody> | ||
| </Table> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and loading state management.
While the component structure is clean and follows React best practices, there's no visible error handling or loading state management. This could lead to a poor user experience if data fetching is slow or fails.
Consider implementing error handling and loading state management. You could use a try-catch block in the parent component or data fetching hook, and pass down an isLoading and error prop to this component. Then, render appropriate UI based on these props:
export const VariableSetsTable: React.FC<{
variableSets: (SCHEMA.VariableSet & {
values: SCHEMA.VariableSetValue[];
assignments: (SCHEMA.VariableSetAssignment & {
environment: SCHEMA.Environment;
})[];
})[];
isLoading?: boolean;
error?: Error;
}> = ({ variableSets, isLoading, error }) => {
const { setVariableSetId } = useVariableSetDrawer();
if (isLoading) {
return <div>Loading variable sets...</div>;
}
if (error) {
return <div>Error loading variable sets: {error.message}</div>;
}
if (variableSets.length === 0) {
return <div>No variable sets found.</div>;
}
// Existing table rendering code...
};This change would provide better feedback to users about the state of the data and handle potential edge cases.
|
|
||
| const schema = z.object({ | ||
| name: z.string().min(1).max(255), | ||
| description: z.string(), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add maximum length validation for 'description' field.
Currently, the description field has no maximum length constraint. To prevent excessively long inputs and potential performance issues, consider adding a maximum length validation.
Suggested change:
- description: z.string(),
+ description: z.string().max(1000),Adjust the maximum length as appropriate for your use case.
📝 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.
| description: z.string(), | |
| description: z.string().max(1000), |
| <Input | ||
| placeholder="Value" | ||
| type="number" | ||
| value={value.value} | ||
| onChange={(e) => { | ||
| const num = e.target.valueAsNumber; | ||
| onChange({ ...value, value: num }); | ||
| }} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Handle NaN values when parsing number inputs.
When parsing number inputs, e.target.valueAsNumber may return NaN if the input is invalid or empty. Assigning NaN to value.value can cause unexpected behavior.
Consider checking for NaN and handling it appropriately:
- const num = e.target.valueAsNumber;
+ const num = e.target.valueAsNumber;
+ if (isNaN(num)) {
+ // Handle NaN, e.g., set a default value or prevent the update
+ return;
+ }
onChange({ ...value, value: num });Alternatively, set a default value:
- const num = e.target.valueAsNumber;
+ const num = isNaN(e.target.valueAsNumber) ? 0 : e.target.valueAsNumber;
onChange({ ...value, value: num });📝 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.
| <Input | |
| placeholder="Value" | |
| type="number" | |
| value={value.value} | |
| onChange={(e) => { | |
| const num = e.target.valueAsNumber; | |
| onChange({ ...value, value: num }); | |
| }} | |
| /> | |
| )} | |
| <Input | |
| placeholder="Value" | |
| type="number" | |
| value={value.value} | |
| onChange={(e) => { | |
| const num = e.target.valueAsNumber; | |
| if (isNaN(num)) { | |
| // Handle NaN, e.g., set a default value or prevent the update | |
| return; | |
| } | |
| onChange({ ...value, value: num }); | |
| }} | |
| /> | |
| )} | |
| ``` | |
| This suggestion implements the first approach mentioned in the review comment, which checks for NaN and prevents the update if the input is invalid. This is a more cautious approach that avoids updating the state with an invalid value. | |
| Note: The review comment also provides an alternative suggestion using a default value. If you prefer that approach, you could use the following code instead: | |
| ```suggestion | |
| <Input | |
| placeholder="Value" | |
| type="number" | |
| value={value.value} | |
| onChange={(e) => { | |
| const num = isNaN(e.target.valueAsNumber) ? 0 : e.target.valueAsNumber; | |
| onChange({ ...value, value: num }); | |
| }} | |
| /> | |
| )} |
| key={field.id} | ||
| name={`environmentIds.${index}`} | ||
| render={({ field: { value } }) => { | ||
| const env = environments.find((e) => e.id === value.id)!; |
There was a problem hiding this comment.
Avoid using non-null assertion operator '!'.
The non-null assertion operator '!' is used in line 270. This can mask potential null or undefined values and lead to runtime errors. It's better to handle the possibility that environments.find(...) may return undefined.
Consider handling the undefined case explicitly:
- const env = environments.find((e) => e.id === value.id)!;
+ const env = environments.find((e) => e.id === value.id);
+ if (!env) {
+ // Handle the case when env is not found
+ return null; // or display an error message
+ }Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 270-270: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
|
|
||
| vi.mocked(utils.getVariableValues).mockResolvedValue(variableValues); | ||
| vi.mocked(utils.getFirstMatchedTarget).mockResolvedValue( | ||
| variableValues[1]!, |
There was a problem hiding this comment.
Avoid using the non-null assertion operator ! throughout the test cases
The non-null assertion operator ! is used in multiple places within your test cases, specifically at lines 125, 138, 173, 208, 232, 275, 276, 300, 343, 408, 473, and 557. Using ! can lead to runtime errors if the value is actually null or undefined. It is recommended to handle potential undefined values explicitly to improve code safety and maintainability.
Consider the following approaches to address this:
-
Use Optional Chaining (
?.): Replace!with?.to safely access properties when the object might beundefinedornull.- expect(result[0]!.value).toEqual(variableValues[1]!.value); + expect(result[0]?.value).toEqual(variableValues[1]?.value);
-
Add Assertions: Explicitly check that the values are not
undefinedbefore using them.expect(result[0]).toBeDefined(); expect(variableValues[1]).toBeDefined(); expect(result[0].value).toEqual(variableValues[1].value);
-
Refactor Test Data: Ensure that your test data guarantees the presence of necessary values.
-
Adjust Type Definitions: If certain that values are not
undefined, adjust your type definitions to reflect this, possibly by using types that enforce non-null values.
Also applies to: 138-138, 173-173, 208-208, 232-232, 275-275, 276-276, 300-300, 343-343, 408-408, 473-473, 557-557
🧰 Tools
🪛 Biome
[error] 125-125: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| it("should return direct match value if it exists", async () => { | ||
| vi.mocked(utils.getJob).mockResolvedValue(job); | ||
|
|
||
| const variables: variable[] = [ | ||
| { | ||
| deployment_variable: { | ||
| id: "0", | ||
| config: { type: "string", inputType: "text" }, | ||
| description: "test", | ||
| deploymentId: "0", | ||
| key: "test", | ||
| defaultValueId: "0", | ||
| }, | ||
| release, | ||
| }, | ||
| ]; | ||
| vi.mocked(utils.getDeploymentVariables).mockResolvedValue(variables); | ||
| vi.mocked(utils.getTarget).mockResolvedValue(target); | ||
|
|
||
| vi.mocked(utils.getVariableValues).mockResolvedValue(variableValues); | ||
| vi.mocked(utils.getFirstMatchedTarget).mockResolvedValue( | ||
| variableValues[1]!, | ||
| ); | ||
| vi.mocked(utils.getEnvironment).mockResolvedValue(undefined); | ||
|
|
||
| const result = await jobVariablesDeployment.determineVariablesForReleaseJob( | ||
| db, | ||
| { | ||
| ...job.job, | ||
| releaseJobTrigger: job.release_job_trigger, | ||
| }, | ||
| ); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(result[0]!.value).toEqual(variableValues[1]!.value); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive mock setups in test cases
The test cases contain repetitive code for setting up mocks and test data. Specifically, the initialization of job, variables, variableValues, target, and the mocking of utility functions are repeated across multiple tests. Refactoring this setup into shared helper functions or using beforeEach hooks can improve code readability and maintainability.
Suggested Refactor:
-
Create Helper Functions: Extract the repeated setup code into helper functions that can be reused.
function setupDefaultMocks() { vi.mocked(utils.getJob).mockResolvedValue(job); vi.mocked(utils.getDeploymentVariables).mockResolvedValue(defaultVariables); vi.mocked(utils.getTarget).mockResolvedValue(target); vi.mocked(utils.getVariableValues).mockResolvedValue(variableValues); }
-
Use
beforeEachHook: Implement abeforeEachhook to apply the common setup before each test.beforeEach(() => { vi.clearAllMocks(); setupDefaultMocks(); });
-
Override Mocks in Specific Tests: For tests that require different mock behaviors, override the mocks within the test case.
it("should return direct match value if it exists", async () => { vi.mocked(utils.getFirstMatchedTarget).mockResolvedValue(variableValues[1]); // Rest of the test code... });
Also applies to: 141-174, 176-209, 211-277, 279-344, 346-409, 411-475, 476-558
🧰 Tools
🪛 Biome
[error] 125-125: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 138-138: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
[error] 138-138: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
| variableSets: (SCHEMA.VariableSet & { | ||
| values: SCHEMA.VariableSetValue[]; | ||
| assignments: (SCHEMA.VariableSetAssignment & { | ||
| environment: SCHEMA.Environment; | ||
| })[]; | ||
| })[]; |
There was a problem hiding this comment.
this needs to be extracted out to its own props var, this is confusing af
| a.variableSet.name.localeCompare(b.variableSet.name), | ||
| ); | ||
|
|
||
| assignments.forEach((assignment) => { |
| if (id === null) { | ||
| url.searchParams.delete(param); | ||
| } else { | ||
| url.searchParams.set(param, id); | ||
| } |
There was a problem hiding this comment.
remove need for else, and == not ===
There was a problem hiding this comment.
Actionable comments posted: 15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (10)
- apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/VariableSetActionsDropdown.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/VariableSetDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/useVariableSetDrawer.ts (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/CreateVariableSetDialog.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/GettingStartedVariableSets.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/VariableSetsTable.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/page.tsx (1 hunks)
- packages/api/src/router/variable-set.ts (5 hunks)
- packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts (1 hunks)
- packages/job-dispatch/src/job-variables-deployment/utils.ts (1 hunks)
🧰 Additional context used
🪛 Biome
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/CreateVariableSetDialog.tsx
[error] 271-271: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts
[error] 99-99: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 146-146: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (10)
apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/useVariableSetDrawer.ts (3)
1-3: LGTM: Imports and constant declaration are appropriate.The imports from Next.js navigation are correctly used, and the
paramconstant is well-defined for URL parameter management.
21-24: LGTM:removeVariableSetIdand hook return are well-structured.The
removeVariableSetIdfunction is a useful convenience method. The hook's return value provides a clean interface for managing the variable set ID in the URL.
1-24: Great job on implementing theuseVariableSetDrawerhook!This custom hook provides a clean and reusable solution for managing variable set IDs in the URL. It effectively utilizes Next.js navigation features and separates concerns by handling URL parameter management in a dedicated hook. This implementation will likely improve the maintainability and consistency of variable set handling across the application.
A few minor suggestions were made to further enhance the code:
- Consider adding type annotations for improved clarity.
- Implement error handling for URL parsing.
Overall, this is a well-structured and valuable addition to the codebase.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/GettingStartedVariableSets.tsx (4)
1-1: LGTM: Import statement added correctly.The new import statement for
SCHEMAfrom "@ctrlplane/db/schema" is correctly added and necessary for the newenvironmentsprop type.
6-6: Good catch: Import statement corrected.The import statement for
CreateVariableSetDialoghas been correctly updated from "./CreateValueSetDialog" to "./CreateVariableSetDialog". This change ensures accurate naming and proper module import.
8-15: Excellent: Type alias implemented and props updated.Great job implementing the suggestion from the previous review. The new
VariableSetGettingStartedPropstype alias improves code readability and maintainability. The component's props have been correctly updated to use this new type, including the addition of theenvironmentsprop.
35-38: LGTM: CreateVariableSetDialog usage updated correctly.The usage of
CreateVariableSetDialoghas been properly updated to include both thesystemIdandenvironmentsprops. This change is consistent with the updates made to the component's props and type definition.apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/VariableSetDrawer.tsx (2)
14-14: Previous comment still applies regarding simplification ofisOpenThe suggestion to simplify the
isOpencondition by using a double negation (!!variableSetId) remains applicable.
15-15: Previous comment still applies regardingonOpenChangehandlerThe issue identified in the previous review about
setIsOpennot matching the expected signature ofonOpenChangeis still present.apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/CreateVariableSetDialog.tsx (1)
1-328: Well-structured implementation of theCreateVariableSetDialogcomponentThe component is well-designed, with clean code and effective use of React hooks and form handling libraries. The validation schema using Zod is comprehensive, ensuring data integrity. The use of UI components enhances readability and maintainability.
🧰 Tools
🪛 Biome
[error] 271-271: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/useVariableSetDrawer.ts
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/useVariableSetDrawer.ts
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/VariableSetDrawer.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/page.tsx
Outdated
Show resolved
Hide resolved
.../webservice/src/app/[workspaceSlug]/systems/[systemSlug]/variable-sets/VariableSetsTable.tsx
Outdated
Show resolved
Hide resolved
...rvice/src/app/[workspaceSlug]/_components/variable-set-drawer/VariableSetActionsDropdown.tsx
Show resolved
Hide resolved
packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts
Outdated
Show resolved
Hide resolved
packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit
Release Notes
New Features
OverviewContentcomponent for managing variable sets with dynamic fields and API integration.VariableSetDrawercomponent for a user-friendly variable set management interface.CreateVariableSetDialogfor creating variable sets with enhanced form validation.VariableSetsTablefor displaying variable sets in a structured table format.VariableSetActionsDropdownfor managing actions related to variable sets, including deletion functionality.SystemVariableSetsPageto include new components and improved data handling.useVariableSetDrawerfor managing variable set ID in URL parameters.Bug Fixes
Chores