fix: Identifier filter and other filter cleanup #180
Conversation
WalkthroughThis pull request encompasses a comprehensive refactor of various components and schemas within the application, transitioning from version-based logic to column-based logic. Key changes include the renaming of types and variables, updates to operator handling across multiple components, and the introduction of new components for managing identifier conditions. The modifications enhance the clarity and consistency of condition handling, ensuring that components utilize the newly defined Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (4)
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: 7
🧹 Outside diff range and nitpick comments (16)
packages/validators/src/targets/conditions/index.ts (1)
6-6: LGTM! Consider maintaining alphabetical order.The addition of the identifier condition export aligns well with the PR objectives. As a minor suggestion, consider moving it between
comparison-condition.jsandkind-condition.jsto maintain alphabetical ordering with the other exports.export * from "./name-condition.js"; export * from "./kind-condition.js"; export * from "./comparison-condition.js"; +export * from "./identifier-condition.js"; export * from "./target-condition.js"; export * from "./provider-condition.js"; -export * from "./identifier-condition.js";packages/validators/src/targets/conditions/identifier-condition.ts (1)
5-9: Add JSDoc documentation for the schema.Consider adding documentation to explain the purpose, usage, and validation rules of this schema. This will help other developers understand how to use it correctly.
+/** + * Zod schema for validating identifier conditions. + * @property {string} type - Must be "identifier" + * @property {ColumnOperator} operator - Valid column comparison operator + * @property {string} value - Non-empty string to compare against + */ export const identifierCondition = z.object({apps/webservice/src/app/[workspaceSlug]/_components/job-condition/VersionConditionRender.tsx (1)
The transition to column-based logic is incomplete and needs attention
The verification reveals that while
VersionConditionRender.tsxhas been updated to useColumnConditionRender, there are still version-specific components and types being used throughout the codebase that should be transitioned to the new column-based approach:
- Both
ReleaseVersionConditionRenderandJobReleaseVersionConditionRendercomponents are using the same column-based implementation but with different names- Version-specific types and conditions are still present in multiple locations:
packages/validators/src/releases/conditions/packages/validators/src/jobs/conditions/packages/db/src/schema/The refactoring should be extended to:
- Consolidate the duplicate version condition render components
- Update the validator and schema layers to use column-based types consistently
🔗 Analysis chain
Line range hint
1-24: LGTM! Changes align well with the column-based logic transition.The implementation correctly transitions from version-based to column-based logic while maintaining type safety and React best practices. All props are properly passed to the new
ColumnConditionRendercomponent.Let's verify that all version-based components have been consistently updated across the codebase:
Let me gather more information about the implementation of these components to verify the consistency of the transition.
Let me check one final aspect about the version-specific types and conditions without the typescript filter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent transition from version-based to column-based components # Test 1: Search for any remaining VersionOperatorType usage echo "Checking for remaining VersionOperatorType usage..." rg "VersionOperatorType" # Test 2: Search for any remaining VersionConditionRender components echo "Checking for remaining VersionConditionRender components..." rg "VersionConditionRender" # Test 3: Verify ColumnOperatorType is used consistently echo "Checking ColumnOperatorType usage in condition components..." rg -A 2 "ColumnOperatorType.*condition"Length of output: 2201
Script:
#!/bin/bash # Check the implementation of ReleaseVersionConditionRender and JobReleaseVersionConditionRender echo "Checking ReleaseVersionConditionRender implementation..." ast-grep --pattern 'export const ReleaseVersionConditionRender: React.FC<$_> = $_' echo "Checking JobReleaseVersionConditionRender implementation..." ast-grep --pattern 'export const JobReleaseVersionConditionRender: React.FC<$_> = $_' # Check if there are any other version-specific operators or conditions still in use echo "Checking for version-specific types and conditions..." rg -A 2 "Version.*Type|Version.*Condition" --type typescriptLength of output: 5393
Script:
#!/bin/bash # Check for version-specific types and conditions in all files echo "Checking for version-specific types and conditions..." rg -A 2 "Version.*Type|Version.*Condition" # Check imports of version-related types echo "Checking version-related type imports..." rg -A 1 "import.*Version.*from.*validators"Length of output: 13398
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseVersionConditionRender.tsx (1)
18-26: Consider enhancing the version title for clarity.The component migration to
ColumnConditionRenderis well-implemented. However, consider making the title more descriptive, such as "Release Version" instead of just "Version", to provide better context to users.- title="Version" + title="Release Version"apps/webservice/src/app/[workspaceSlug]/_components/target-condition/IdentifierConditionRender.tsx (1)
7-9: Consider adding runtime prop validation.While the TypeScript types provide compile-time safety, consider adding runtime prop validation using PropTypes or a schema validation library to catch potential issues during development and testing.
Example implementation:
import PropTypes from 'prop-types'; // After the component definition IdentifierConditionRender.propTypes = { condition: PropTypes.shape({ operator: PropTypes.string.isRequired, value: PropTypes.string.isRequired }).isRequired, onChange: PropTypes.func.isRequired, className: PropTypes.string };packages/validators/src/conditions/index.ts (2)
12-17: Add JSDoc comments to document operator behaviors.Consider adding JSDoc comments to document the expected behavior and use cases for each operator in the
ColumnOperatorenum. This would help other developers understand when to use each operator.export enum ColumnOperator { + /** Exact match comparison */ Equals = "equals", + /** Case-sensitive substring match */ Like = "like", + /** Regular expression pattern match */ Regex = "regex", }
33-35: Consider alternative patterns for initialization order.The current solution of keeping the schema here due to initialization order issues could be improved. Consider using a factory pattern or lazy initialization to resolve the circular dependency.
Example approach:
// In a separate file (e.g., operators.ts) export const createVersionCondition = (columnOperator: z.ZodType) => z.object({ type: z.literal("version"), operator: columnOperator, value: z.string().min(1), });packages/validators/src/targets/conditions/comparison-condition.ts (1)
Line range hint
1-44: Consider adding JSDoc documentation for the schema and type.While the implementation is correct, adding JSDoc documentation would improve maintainability by explaining:
- The purpose of the comparison condition
- The role of the identifier condition in the comparison
- Examples of valid condition structures
Here's a suggested documentation:
+/** + * Schema for validating comparison conditions that combine multiple conditions with logical operators. + * Supports metadata, kind, name, provider, and identifier conditions. + * @example + * { + * type: "comparison", + * operator: "and", + * conditions: [ + * { type: "identifier", ... }, + * { type: "metadata", ... } + * ] + * } + */ export const comparisonCondition: z.ZodType<ComparisonCondition> = z.lazy(() => +/** + * Type definition for a comparison condition that combines multiple conditions with logical operators. + * @property {string} type - Always "comparison" + * @property {("and"|"or")} operator - Logical operator to combine conditions + * @property {boolean} [not] - Optional flag to negate the entire comparison + * @property {Array<Condition>} conditions - Array of conditions to combine + */ export type ComparisonCondition = {apps/webservice/src/app/[workspaceSlug]/_components/filter/ColumnConditionRender.tsx (2)
14-21: Consider strengthening type definitions.The props type could be more specific to improve type safety:
valuecould be constrained to a more specific string type based on the operatorclassNamecould use a more semantic type likeReact.HTMLAttributes<HTMLDivElement>['className']type ColumnConditionRenderProps = { operator: ColumnOperatorType; - value: string; + value: string; // TODO: Consider union type based on operator setOperator: (operator: ColumnOperatorType) => void; setValue: (value: string) => void; title: string; - className?: string; + className?: React.HTMLAttributes<HTMLDivElement>['className']; };
Line range hint
31-63: Consider these UI/UX improvements.
- The grid layout uses hardcoded column spans which might not be responsive on smaller screens
- The placeholder logic could be extracted to a separate function for better maintainability
- The
cursor-pointerclass on the Input component seems unnecessary for a text inputConsider this refactoring:
+const getPlaceholder = (operator: ColumnOperatorType): string => { + switch (operator) { + case ColumnOperator.Regex: + return "^[a-zA-Z]+$"; + case ColumnOperator.Like: + return "%value%"; + default: + return "Value"; + } +}; export const ColumnConditionRender: React.FC<ColumnConditionRenderProps> = ({ // ...props }) => ( - <div className="grid w-full grid-cols-12"> + <div className="grid w-full grid-cols-12 md:grid-cols-12 sm:grid-cols-1"> {/* ... */} <div className="col-span-7"> <Input - placeholder={ - operator === ColumnOperator.Regex - ? "^[a-zA-Z]+$" - : operator === ColumnOperator.Like - ? "%value%" - : "Value" - } + placeholder={getPlaceholder(operator)} value={value} onChange={(e) => setValue(e.target.value)} - className="w-full cursor-pointer rounded-l-none" + className="w-full rounded-l-none" /> </div> </div> );apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionRender.tsx (2)
83-90: Consider reordering conditions for better maintainability.The identifier condition check is added at the end of all conditions. Consider grouping similar conditions together (e.g., all identifier-related conditions) to improve code organization and maintainability.
Consider moving this condition block next to similar condition checks (e.g., near
isNameConditionor other identifier-related conditions) to maintain logical grouping.
Line range hint
22-25: Update component documentation.The component's documentation comment about minimum width requirement could be enhanced to explain why this requirement exists and how it affects the new identifier condition rendering.
Consider expanding the documentation to include:
- Rationale for the minimum width requirement
- Impact on different condition types including the new identifier condition
- Any specific layout considerations for implementers
packages/validators/src/releases/conditions/release-condition.ts (1)
Line range hint
13-17: Consider deprecating version-based types and functions.Given the transition from version-based to column-based logic mentioned in the PR summary, consider:
- Marking version-related types and functions as deprecated
- Adding migration documentation for users
- Planning the complete removal of version-based logic in a future PR
This will help users transition to the new column-based approach smoothly.
Also applies to: 71-73
packages/db/src/schema/release.ts (1)
233-236: Consider using ColumnOperator.Regex for consistency.While the transition to
ColumnOperatorenum improves type safety, the regex case should also use an enum value for consistency.Consider updating the regex case:
if (cond.operator === ColumnOperator.Equals) return eq(release.version, cond.value); if (cond.operator === ColumnOperator.Like) return like(release.version, cond.value); - return sql`${release.version} ~ ${cond.value}`; + if (cond.operator === ColumnOperator.Regex) + return sql`${release.version} ~ ${cond.value}`; + throw new Error(`Unsupported version operator: ${cond.operator}`);apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionBadge.tsx (1)
179-189: Consider component abstraction to reduce duplication.The implementation is correct and consistent with other condition components. However, there's an opportunity to reduce code duplication since
StringifiedIdentifierConditionandStringifiedNameConditionshare identical structure and behavior.Consider creating a shared base component:
const StringifiedBasicCondition: React.FC<{ condition: { operator: TargetOperator; value: string }; label: string; }> = ({ condition, label }) => ( <ConditionBadge> <span className="text-white">{label}</span> <span className="text-muted-foreground"> {operatorVerbs[condition.operator]} </span> <span className="text-white">{condition.value.replace(/%/g, "")}</span> </ConditionBadge> ); // Then reuse it: const StringifiedIdentifierCondition: React.FC<{ condition: IdentifierCondition; }> = ({ condition }) => ( <StringifiedBasicCondition condition={condition} label="Identifier" /> );packages/db/src/schema/target.ts (1)
205-212: Handle unexpectedTargetFilterTypevalues explicitlyIn the
buildConditionfunction, variouscond.typecases are handled. However, ifcond.typeis an unexpected value, the function may proceed without proper handling. Consider adding an explicit else clause or throwing an error to handle unsupportedTargetFilterTypevalues, improving code robustness and readability.Apply this diff to add error handling:
if (cond.type === TargetFilterType.Identifier) return buildIdentifierCondition(tx, cond); +else + throw new Error(`Unsupported TargetFilterType: ${cond.type}`);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (25)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/ColumnConditionRender.tsx (3 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionBadge.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/VersionConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ComparisonConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseVersionConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ComparisonConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/IdentifierConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionBadge.tsx (4 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionRender.tsx (3 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (2 hunks)
- packages/db/src/schema/job.ts (5 hunks)
- packages/db/src/schema/release.ts (5 hunks)
- packages/db/src/schema/target.ts (6 hunks)
- packages/validators/src/conditions/index.ts (2 hunks)
- packages/validators/src/conditions/version-condition.ts (0 hunks)
- packages/validators/src/jobs/conditions/comparison-condition.ts (1 hunks)
- packages/validators/src/jobs/conditions/job-condition.ts (2 hunks)
- packages/validators/src/releases/conditions/comparison-condition.ts (1 hunks)
- packages/validators/src/releases/conditions/release-condition.ts (1 hunks)
- packages/validators/src/targets/conditions/comparison-condition.ts (3 hunks)
- packages/validators/src/targets/conditions/identifier-condition.ts (1 hunks)
- packages/validators/src/targets/conditions/index.ts (1 hunks)
- packages/validators/src/targets/conditions/target-condition.ts (4 hunks)
💤 Files with no reviewable changes (1)
- packages/validators/src/conditions/version-condition.ts
✅ Files skipped from review due to trivial changes (1)
- packages/validators/src/releases/conditions/comparison-condition.ts
🔇 Additional comments (46)
packages/validators/src/targets/conditions/identifier-condition.ts (3)
1-3: LGTM! Imports are correctly defined.The necessary dependencies are imported, and the
.jsextension is correctly used for local imports in an ESM context.
1-15: Verify integration with the existing condition system.Let's verify that this new condition type is properly integrated with the existing condition system.
#!/bin/bash # Description: Check integration points with the existing condition system # Test 1: Check if the condition is exported from the conditions index rg "export.*identifier-condition" "packages/validators/src/targets/conditions/index.ts" # Test 2: Check if IdentifierCondition is included in union types rg "IdentifierCondition.*\|" "packages/validators/src/targets/"
7-7: Verify the imported columnOperator schema.Let's verify that the imported columnOperator schema contains appropriate operators for identifier comparisons.
✅ Verification successful
The columnOperator schema is properly defined with appropriate operators for identifier comparisons
The columnOperator schema is correctly defined in
conditions/index.tsand includes three appropriate operators for identifier comparisons:
equals: For exact matcheslike: For pattern matchingregex: For regular expression matching🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the definition of columnOperator to ensure it has appropriate operators # Test: Search for columnOperator definition rg -A 10 "export const columnOperator = " "packages/validators/src/conditions/"Length of output: 194
Script:
#!/bin/bash # Let's try to find the columnOperator definition with a broader search rg "columnOperator" packages/validators/src/ -A 5 # Also search for any operator-related type definitions ast-grep --pattern 'type $_ = { operator: $_ }' # Let's also check imports in the identifier-condition file rg "import.*columnOperator" packages/validators/src/Length of output: 2700
Script:
#!/bin/bash # Let's find the ColumnOperator enum definition rg "enum ColumnOperator" packages/validators/src/ -A 5Length of output: 409
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseVersionConditionRender.tsx (2)
1-4: LGTM: Import changes align with column-based transition.The addition of
ColumnOperatorTypewhile retainingVersionConditioncorrectly supports the transition to column-based logic while maintaining type safety for version-specific conditions.
13-14: Verify operator type compatibility across usages.The operator type change looks good, but let's verify that all possible values of
condition.operatorare compatible withColumnOperatorType.✅ Verification successful
Let me gather more specific information about the operator types and their usage.
Let me check the specific implementation of ColumnOperatorType to verify compatibility.
Let me check one final piece - the ColumnOperator enum definition to ensure complete type compatibility.
Type compatibility verified for operator changes
The operator type change is safe and correct. The
setOperatorfunction inReleaseVersionConditionRendercorrectly usesColumnOperatorType, which is derived from theColumnOperatorenum containing exactly the expected values: "equals", "like", and "regex". This matches the type used in theVersionConditiontype and maintains type safety throughout the component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining version operator references and verify column operator usage # Expected: No references to version operators in the codebase # Search for any remaining version operator references rg -i 'versionoperator' # Search for column operator usage in condition contexts ast-grep --pattern 'condition.operator'Length of output: 29195
Script:
#!/bin/bash # Search for operator type definitions and their usage in the codebase # This will help verify compatibility between ColumnOperatorType and other operator types # Search for operator type definitions rg -A 5 "type.*Operator.*=" # Search for specific operator imports and usage in the ReleaseVersionConditionRender component rg -l "ColumnOperatorType"Length of output: 2818
Script:
#!/bin/bash # Check the implementation of ColumnOperatorType and its usage in ReleaseVersionConditionRender # Get the definition of ColumnOperatorType rg -A 10 "const columnOperator = " # Get the implementation of ReleaseVersionConditionRender cat "apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseVersionConditionRender.tsx"Length of output: 1732
Script:
#!/bin/bash # Get the definition of ColumnOperator enum rg -A 5 "enum ColumnOperator" # Also check if VersionCondition type uses the same operator type rg -A 5 "type VersionCondition ="Length of output: 529
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/IdentifierConditionRender.tsx (1)
1-6: LGTM! Well-structured imports.The imports are properly organized, using type imports where appropriate and maintaining clear separation between external and internal dependencies.
packages/validators/src/conditions/index.ts (2)
36-42: LGTM! Well-structured schema definition.The version condition schema is well-defined with proper validation:
- Enforces literal type checking
- Uses the column operator validation
- Ensures non-empty string values
1-4: Verify complete transition from version-based logic.While the
version-condition.jsexport has been removed, we still have version-related code in this file. Let's verify that this is intentional and that no other files still depend on the removed export.packages/validators/src/targets/conditions/comparison-condition.ts (3)
4-4: LGTM: Import statements are properly organized.The imports for both the type and schema of
IdentifierConditionare correctly added, maintaining consistency with the existing pattern of importing both types and their corresponding schemas.Also applies to: 9-9
42-42: LGTM: Type definition is properly updated.The
IdentifierConditiontype is correctly added to the union type, ensuring type safety at compile time.
26-26: LGTM: Schema union is correctly extended.The
identifierConditionschema is properly added to the union of conditions, maintaining type safety through Zod's runtime validation.Let's verify the schema implementation:
✅ Verification successful
Schema implementation and integration verified successfully
The
identifierConditionschema is correctly implemented using Zod and properly integrated into thecomparisonConditionunion. The schema defines:
- A literal "identifier" type
- An operator using the imported
columnOperator- A non-empty string value
- Proper type exports using Zod inference
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the identifier-condition.ts file exists and exports the expected schema # Expected: File exists and exports both the type and schema # Check if the file exists and contains the exports rg -l "export const identifierCondition.*=.*z\." "identifier-condition.ts" rg -l "export (type|interface) IdentifierCondition" "identifier-condition.ts"Length of output: 280
Script:
#!/bin/bash # Let's search for the identifier-condition file in the entire repository fd "identifier-condition" --type f # Let's also check the comparison-condition.ts file to understand the context fd "comparison-condition.ts" --type f --exec cat {}Length of output: 4125
Script:
#!/bin/bash # Let's check the identifier-condition.ts file to verify its schema implementation cat packages/validators/src/targets/conditions/identifier-condition.tsLength of output: 457
packages/validators/src/jobs/conditions/comparison-condition.ts (3)
6-6: LGTM: Import consolidation improves maintainabilityThe consolidation of imports through the index file aligns with best practices for better maintainability and easier refactoring.
Also applies to: 15-15
Line range hint
23-35: LGTM: Schema definition is consistent with typesThe addition of
versionConditionto the schema's union type is properly implemented and maintains consistent ordering.
Line range hint
37-53: LGTM: Type definition aligns with schema changesThe addition of
VersionConditionto the union type is consistent with the schema definition. However, since this is part of a broader refactor from version-based to column-based logic, let's verify the integration.✅ Verification successful
LGTM: Version condition integration is properly implemented
The verification shows that the
VersionConditiontype is well-integrated across the codebase:
- Properly imported and used in both job and release condition schemas
- Implemented in the database layer with column-based operators (Like, Equals)
- Has corresponding UI components for rendering and handling version conditions
- Type guards and condition builders are in place for proper type safety
The transition to column-based logic is complete and consistent across the validators, database, and UI layers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the transition from version-based to column-based logic # Expected: No remaining references to version-specific operators or old version condition imports # Check for any remaining version-specific operators echo "Checking for version-specific operators..." rg -g '!*.{md,json}' "VersionOperator" # Check for direct imports from version-condition file echo "Checking for direct version-condition imports..." rg -g '!*.{md,json}' "from ['\"].*version-condition['\"]"Length of output: 304
Script:
#!/bin/bash # Let's try a different approach to verify the transition # 1. Find all files that might contain version-related code # 2. Check the version condition implementation # 3. Look for any version-related imports or types # Find the version condition implementation echo "Version condition implementation:" ast-grep --pattern 'type VersionCondition = { $$$ }' # Check for any version-related imports echo -e "\nVersion-related imports:" rg -g '!*.{md,json}' 'import.*Version.*from' # Look for version-related type usage echo -e "\nVersion type usage:" rg -g '!*.{md,json}' 'Version(Condition|Operator)' -A 2Length of output: 13802
apps/webservice/src/app/[workspaceSlug]/_components/filter/ColumnConditionRender.tsx (2)
23-30: LGTM! Clean component declaration.The component declaration follows React best practices with proper type annotations and clean props destructuring.
23-23: Verify component name change across the codebase.Since this component has been renamed from
VersionConditionRendertoColumnConditionRender, let's verify that all imports have been updated.✅ Verification successful
Component rename verification completed successfully
The search results show that:
- The
ColumnConditionRendercomponent is properly imported and used in multiple components (IdentifierConditionRender,ReleaseVersionConditionRender, andVersionConditionRender)- The occurrences of "VersionConditionRender" in the codebase are different components (
ReleaseVersionConditionRenderandJobReleaseVersionConditionRender) and not remnants of the old component name🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old component name rg "VersionConditionRender" --type tsx --type ts # Search for imports of the new component name to ensure it's being used rg "ColumnConditionRender" --type tsx --type tsLength of output: 152
Script:
#!/bin/bash # Search for any remaining references to the old component name without file type restrictions rg "VersionConditionRender" # Search for imports of the new component name rg "ColumnConditionRender"Length of output: 2145
packages/validators/src/releases/conditions/release-condition.ts (2)
10-10: LGTM! Import consolidation is consistent.The consolidation of condition imports aligns with the type imports and improves code organization.
#!/bin/bash # Description: Verify that the imported conditions are properly used # Test: Check usage of metadataCondition and versionCondition rg -A 2 "metadataCondition|versionCondition" --type typescript
4-7: Verify complete migration of VersionCondition.The import consolidation looks good, but let's verify that the
version-condition.jsfile has been properly migrated and removed.✅ Verification successful
Import consolidation is correct and properly implemented
The verification shows that
VersionConditiontype and its schema are directly defined inconditions/index.ts. The import consolidation in the review is correct as there's a comment inindex.tsexplicitly stating why the version condition needs to be defined there due to initialization dependencies withcolumnOperator.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that version-condition.js has been removed and all its contents properly migrated # Test 1: Check if version-condition.js still exists fd "version-condition.js" # Test 2: Verify VersionCondition is properly exported from index.js rg -l "export.*VersionCondition" "conditions/index.js"Length of output: 349
Script:
#!/bin/bash # Let's try a more comprehensive search approach # Search for any file containing VersionCondition rg -l "VersionCondition" # Search for files with "condition" in their name fd "condition" # Look for exports of VersionCondition in any file rg "export.*VersionCondition" # Search for type definitions of VersionCondition ast-grep --pattern 'type VersionCondition = $_'Length of output: 6431
Script:
#!/bin/bash # Let's verify the content of version-condition.ts and index.ts to ensure proper migration # Check the content of version-condition.ts cat packages/validators/src/conditions/version-condition.ts # Check the content of index.ts cat packages/validators/src/conditions/index.ts # Check if version-condition.ts is imported in index.ts rg -A 2 "from.*version-condition" packages/validators/src/conditions/index.tsLength of output: 1228
packages/validators/src/targets/conditions/target-condition.ts (6)
5-5: LGTM: Import statements are properly structured.The new imports for identifier-related modules follow the established pattern and maintain consistency with other condition imports.
Also applies to: 11-11
21-22: LGTM: Type definition is properly extended.The
IdentifierConditionis correctly added to theTargetConditionunion type, maintaining alphabetical ordering and consistent formatting.
30-30: LGTM: Schema validation is properly updated.The
identifierConditionschema is correctly added to thetargetConditionunion schema, maintaining consistency with the type system.
46-46: LGTM: Enum is properly extended.The
Identifiervalue is correctly added to theTargetFilterTypeenum, maintaining alphabetical ordering and consistent naming conventions.
102-105: LGTM: Type guard function is properly implemented.The
isIdentifierConditiontype guard follows the established pattern and correctly implements the type predicate.
115-115: Verify the validation logic for all condition types.The simplified validation logic assumes all non-metadata conditions have a
valueproperty. Let's verify this assumption.✅ Verification successful
The validation logic is safe and correct for all condition types
Based on the search results, all non-metadata condition types in the target conditions (
KindCondition,NameCondition,ProviderCondition,IdentifierCondition) have a mandatoryvalueproperty of typestringwith a minimum length of 1. This is enforced through their Zod schemas:
kindCondition:value: z.string().min(1)nameCondition:value: z.string().min(1)providerCondition:value: z.string().min(1)identifierCondition:value: z.string().min(1)Therefore, checking
condition.value.length > 0is a valid validation as all these conditions require a non-empty string value.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all non-metadata condition types have a 'value' property # and that this property is always a string array or string. # Search for condition type definitions echo "Searching for condition type definitions..." ast-grep --pattern 'export type $condition = { type: TargetFilterType.$_; $$$ value: $_; $$$ }' # Search for condition schema definitions to verify value validation echo "Searching for condition schema definitions..." rg -A 5 'export const \w+Condition = z\.object\({'Length of output: 501
Script:
#!/bin/bash # Let's try a different approach to find condition types and their validation # Search for condition type definitions echo "=== Condition Type Definitions ===" rg -A 10 "export\s+type.*Condition\s*=\s*{" --type typescript # Search for condition schemas with fixed regex echo -e "\n=== Condition Schemas ===" rg -A 10 "export const.*Condition = z\.object" --type typescript # Search for usage of value property in conditions echo -e "\n=== Value Property Usage ===" rg "value:" --type typescript packages/validators/src/targets/conditions/Length of output: 621
Script:
#!/bin/bash # Let's try without typescript file type and escape the curly brace # Search for condition type definitions echo "=== Condition Type Definitions ===" rg -A 10 'export type.*Condition = \{' # Search for condition schemas echo -e "\n=== Condition Schemas ===" rg -A 10 'export const.*Condition = z.object' # Search for value property usage echo -e "\n=== Value Property Usage ===" rg "value:" packages/validators/src/targets/conditions/Length of output: 14015
packages/validators/src/jobs/conditions/job-condition.ts (5)
6-6: LGTM: Import changes are well-organizedThe addition of
VersionConditiontype andversionConditionimports from the consolidatedindex.jsfollows good module organization practices.Also applies to: 20-20
Line range hint
29-35: LGTM: Type definition changes are consistentThe
VersionConditiontype is properly integrated into theJobConditionunion type, maintaining a clean and organized type hierarchy.
Line range hint
37-44: LGTM: Schema changes maintain type-runtime consistencyThe addition of
versionConditionto the Zod union schema ensures proper runtime validation that matches the type system changes.
Line range hint
1-95: Verify impact of version to column-based logic transitionThe AI summary mentions transitioning from version-based to column-based logic. Let's verify that this change doesn't break existing version handling code.
✅ Verification successful
Version condition implementation is consistent and properly integrated
The verification shows that the version condition is properly implemented and integrated:
- Version condition is defined with column operator in
conditions/index.ts- It's consistently used in both job and release conditions
- The implementation maintains proper type safety through TypeScript types
- Version handling is properly validated through
isValidReleaseCondition- No TODO/FIXME comments indicating pending version-related changes
The transition to column-based logic appears to be complete and consistent across the codebase, with no signs of breaking changes to existing version handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for existing version-related code that might be affected # Expected: Find related version handling code to assess impact # Search for version-related conditions and handlers rg -l 'version.*condition|condition.*version' --type ts # Search for any TODO or FIXME comments related to version handling rg -i 'todo.*version|fixme.*version' --type tsLength of output: 239
Script:
#!/bin/bash # Let's examine the version condition usage and implementation # Check the version condition implementation ast-grep --pattern 'export const versionCondition = $_' # Look for version-related types and interfaces ast-grep --pattern 'export (type|interface) $_Version' # Check for version-related imports and usages rg -A 5 'import.*version|from.*version' --type ts # Check the actual implementation in the found files cat packages/validators/src/releases/conditions/release-condition.ts cat packages/validators/src/releases/conditions/comparison-condition.tsLength of output: 6982
Line range hint
89-91: Verify FilterType.Version enum valueThe type guard implementation looks correct, but let's verify that
FilterType.Versionis properly defined in the imported types.✅ Verification successful
FilterType.Version is properly defined and used correctly
The verification confirms that:
FilterType.Versionis properly defined in the conditions module (packages/validators/src/conditions/index.ts)FilterTypeis correctly imported and used in the job conditions module- The
isVersionConditiontype guard correctly usesFilterType.Versionfor type checking🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify FilterType.Version is defined in the conditions module # Expected: Find the Version enum value definition # Search for FilterType enum definition rg -A 10 "export enum FilterType"Length of output: 239
Script:
#!/bin/bash # Let's try different approaches to find FilterType enum definition # Search for FilterType in all TypeScript files rg "FilterType" --type ts -B 2 -A 5 # Also search for any enum containing Version rg "enum.*\{[^}]*Version" --type ts -A 5 # Search for files containing JobFilterType specifically rg "JobFilterType" --type ts -B 2 -A 5Length of output: 90454
Script:
#!/bin/bash # Let's check the FilterType enum definition in the conditions module rg "FilterType.*=.*Version" --type ts -B 2 -A 2 # Also check the import of FilterType in the job-condition file rg "import.*FilterType.*from.*conditions" --type ts -A 3Length of output: 108
Script:
#!/bin/bash # Let's look at the conditions/index.ts file directly since we saw it in previous results cat packages/validators/src/conditions/index.ts # Also check the imports in job-condition.ts cat packages/validators/src/jobs/conditions/job-condition.tsLength of output: 5137
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx (1)
178-178: Consider handling data transformation at the data layer instead of the UI layer.While the change removes '%' characters from version values, performing data transformations at the UI level might mask underlying data quality issues. Consider these architectural improvements:
- Move the transformation logic to a data processing layer
- Add type validation for
condition.value- Document why '%' characters need to be removed
Example implementation at the data layer:
// types/version.ts export interface VersionCondition { value: string; operator: VersionOperator; } // utils/version.ts export const sanitizeVersionValue = (value: string): string => { return value.replace(/%/g, ''); }; // Then in the component: <span className="text-white">{sanitizeVersionValue(condition.value)}</span>Let's verify if this pattern of removing '%' is consistent across other components:
packages/db/src/schema/job.ts (3)
35-39: LGTM: Clean operator type imports.The new imports align well with the transition from version-based to column-based logic, improving the overall clarity of the filtering system.
Line range hint
142-180: LGTM: Well-structured metadata condition handling.The implementation properly handles all metadata operator types (Null, Regex, Like) with appropriate SQL query construction. The code maintains good error handling through type checking and follows consistent patterns.
204-207:⚠️ Potential issueSecurity: Validate regex patterns before using in SQL queries.
The current implementation might be vulnerable to SQL injection or ReDoS attacks through malicious regex patterns. Consider adding safety measures:
- Validate regex patterns before using them in queries
- Implement a timeout mechanism for regex operations
- Consider using parameterized queries instead of raw SQL templates
Let's check if there are any regex pattern validations elsewhere in the codebase:
Consider adding pattern validation:
const buildVersionCondition = (cond: VersionCondition): SQL => { if (cond.operator === ColumnOperator.Like) return like(release.version, cond.value); - if (cond.operator === ColumnOperator.Regex) - return sql`${release.version} ~ ${cond.value}`; + if (cond.operator === ColumnOperator.Regex) { + try { + // Validate regex pattern + new RegExp(cond.value); + return sql`${release.version} ~ ${cond.value}`; + } catch (e) { + throw new Error(`Invalid regex pattern: ${cond.value}`); + } + } return eq(release.version, cond.value); };packages/db/src/schema/release.ts (2)
35-37: LGTM! Good transition to typed enums.The switch from string literals to proper enums (
ColumnOperatorandMetadataOperator) improves type safety and maintainability.
Line range hint
168-195: LGTM! Verify operator coverage.The transition to
MetadataOperatorenum improves type safety while maintaining the existing logic.Let's verify we're handling all possible metadata operators:
✅ Verification successful
All metadata operators are properly handled
The verification confirms that all
MetadataOperatorenum values are covered in the code:
MetadataOperator.Null→ handled withnotExistsMetadataOperator.Regex→ handled withexistsand regex matchMetadataOperator.Like→ handled withexistsand LIKE matchMetadataOperator.Equals→ implicitly handled as the default case (since it's optional in the schema)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all MetadataOperator enum values are handled in buildMetadataCondition # Test: Search for MetadataOperator enum definition and compare with usage echo "=== MetadataOperator enum definition ===" rg --type typescript "export enum MetadataOperator" -A 10 echo -e "\n=== MetadataOperator usage in buildMetadataCondition ===" rg --type typescript "MetadataOperator\." packages/db/src/schema/release.tsLength of output: 546
Script:
#!/bin/bash # Let's try a different approach using basic grep commands # Find the MetadataOperator enum definition echo "=== MetadataOperator enum definition ===" find . -type f -name "*.ts" -exec grep -l "enum MetadataOperator" {} \; | xargs cat # Find operator usage in release.ts echo -e "\n=== MetadataOperator usage in release.ts ===" find . -type f -name "release.ts" -exec grep -A 1 "MetadataOperator\." {} \;Length of output: 2203
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionBadge.tsx (3)
4-4: LGTM: Import additions are well-organized.The new imports for
IdentifierConditiontype andisIdentifierConditionfunction are properly grouped with related imports and follow the established naming patterns.Also applies to: 22-22
236-237: LGTM: Clean integration of identifier condition handling.The integration follows the established pattern and is properly placed in the sequence of condition checks.
236-238: Verify integration with related components.The changes look good, but let's verify the integration with related components mentioned in the summary.
✅ Verification successful
Integration is properly implemented across components
The verification shows that the identifier condition handling is correctly integrated:
StringifiedIdentifierConditionis properly used inTargetConditionBadge.tsxisIdentifierConditionis consistently used in bothTargetConditionBadge.tsxandTargetConditionRender.tsx- The component has proper imports and type definitions from
@ctrlplane/validators/targets- There's a dedicated
IdentifierConditionRender.tsxcomponent that's properly imported and used in the target condition systemThe changes are complete and properly integrated with all related components in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of identifier condition handling across components # Test 1: Check for IdentifierConditionRender component usage echo "Checking for IdentifierConditionRender usage..." rg -l "IdentifierConditionRender" --type tsx # Test 2: Verify consistent operator handling echo "Checking for consistent operator handling..." rg "TargetOperator" --type tsx -A 2 -B 2 # Test 3: Look for any potential missing identifier condition handling echo "Checking for potential missing identifier condition handling..." rg "isIdentifierCondition" --type tsxLength of output: 548
Script:
#!/bin/bash # Let's try without the --type flag since tsx extension might not be recognized # Test 1: Check for StringifiedIdentifierCondition component usage echo "Checking for StringifiedIdentifierCondition usage..." rg "StringifiedIdentifierCondition" # Test 2: Check for isIdentifierCondition usage echo "Checking for isIdentifierCondition usage..." rg "isIdentifierCondition" # Test 3: Look for any identifier condition related imports echo "Checking for identifier condition related imports..." rg "import.*Identifier" -A 2 # Test 4: Find all files that might be related to target conditions echo "Finding target condition related files..." fd "condition" --extension tsx --extension tsLength of output: 7458
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionBadge.tsx (1)
242-242: Verify the impact of removing % signs from version values.The removal of % signs from version values could affect version comparison logic elsewhere in the codebase.
Let's check for any version comparison logic that might depend on these % characters:
✅ Verification successful
Let me check the version comparison logic to ensure we have a complete understanding.
Let me check the version condition implementation to understand how these values are used in comparisons.
Let me check the version condition specific implementation.
Based on the analysis of the codebase, I can now provide the final response:
Removing % signs from version values is safe in this context
The version condition implementation shows that versions are handled as simple string comparisons using basic operators (equals, like, regex) without any special treatment of % characters. The
JobConditionBadgecomponent is purely for display purposes and doesn't affect the underlying version comparison logic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for version comparison logic in the codebase echo "Searching for version-related files and comparisons..." rg -l "version" | rg -i "compar|valid|check" # Search for % usage in version-related code echo "Checking for % usage in version-related code..." rg "%" | rg -i "version"Length of output: 1105
Script:
#!/bin/bash # Check the comparison condition implementations echo "Checking comparison condition implementations..." cat packages/validators/src/releases/conditions/comparison-condition.ts cat packages/validators/src/jobs/conditions/comparison-condition.ts # Check the specific JobConditionBadge usage context echo "Checking JobConditionBadge context..." cat "apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionBadge.tsx"Length of output: 12051
Script:
#!/bin/bash # Check the version condition implementation echo "Checking version condition implementation..." cat packages/validators/src/conditions/index.ts # Also check if there are any version-related utilities echo "Checking for version-related utilities..." fd version packages/validators/src -t fLength of output: 1376
Script:
#!/bin/bash # Check the version condition specific implementation echo "Checking version condition specific implementation..." cat packages/validators/src/conditions/version-condition.ts # Also check for any version pattern matching or comparison usage echo "Checking for version pattern usage..." rg -A 3 "version.*pattern|version.*match|version.*compar" packages/validators/src/Length of output: 348
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (2)
29-29: Verify complete transition to ColumnOperator.The addition of
ColumnOperatorimport aligns with the PR's objective. However, we should verify ifReleaseOperatoris still needed since we're transitioning away from it.✅ Verification successful
Both operators are currently needed - import is correct
The codebase search reveals that both
ReleaseOperatorandColumnOperatorare actively used in the file:
ReleaseOperator.Andis used for the top-level filter combinationColumnOperator.Equalsis used for the version comparisonThis is a valid transitional state where
ReleaseOperatorhandles logical operations (AND/OR) whileColumnOperatorhandles the actual comparisons. The import of both operators is necessary and correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of ReleaseOperator in the codebase rg "ReleaseOperator\." --type ts --type tsxLength of output: 195
Script:
#!/bin/bash # Search for ReleaseOperator usage with correct file type specification rg "ReleaseOperator\." -t typescript -t tsx # Also search for the file content to understand the context better cat "apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx" # Search for any imports of ReleaseOperator rg "import.*ReleaseOperator" -t typescript -t tsxLength of output: 9596
Script:
#!/bin/bash # Try with different file extensions for ripgrep rg "ReleaseOperator\." --type-add 'ts:*.{ts,tsx}' -t ts # Search for actual usage of both operators in the file rg "operator: (ReleaseOperator|ColumnOperator)\." --type-add 'ts:*.{ts,tsx}' -t tsLength of output: 5338
134-137: Verify API compatibility with ColumnOperator.The change from
ReleaseOperator.EqualstoColumnOperator.Equalslooks correct, but we should ensure:
- The
release.list.useQueryendpoint is updated to handleColumnOperator- The version comparison logic remains compatible with this operator change
✅ Verification successful
Based on the code analysis, I can now provide a definitive response about the operator change from
ReleaseOperator.EqualstoColumnOperator.Equals.The operator change is correct and consistent with the codebase.
The codebase shows that:
ColumnOperator.Equalsis the correct operator for version comparisons, as seen inpackages/db/src/schema/release.tswherebuildVersionConditionfunction explicitly handlesColumnOperator.Equalsfor version comparisons.- The version filter type is properly handled in the database layer with
ReleaseFilterType.VersionandColumnOperatorcombinations.- The change aligns with the validator definitions in
packages/validators/src/conditions/index.tswhereColumnOperatoris defined for such comparisons.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the API endpoint definition to verify operator handling ast-grep --pattern 'export const list = procedure .input($_) .query(async ({ input }) => { $$$ })' # Check for any version-specific handling in the codebase rg "ReleaseFilterType.Version" --type ts --type tsx -A 5Length of output: 195
Script:
#!/bin/bash # Search for the API endpoint definition with a more flexible pattern ast-grep --pattern 'export const list = $_' # Search for version-specific handling with correct file extensions rg "ReleaseFilterType.Version" -t ts -t typescript -A 5 # Look for ColumnOperator usage to verify the transition rg "ColumnOperator" -t ts -t typescript -A 5 # Search for release-related router/endpoint definitions fd "release.*router.ts" --type f --exec cat {}Length of output: 267
Script:
#!/bin/bash # Search for files with .ts extension and look for patterns find . -type f -name "*.ts" -o -name "*.tsx" | while read -r file; do echo "=== $file ===" grep -A 5 "ReleaseFilterType\|ColumnOperator\|export const list" "$file" 2>/dev/null done # Find and examine router files specifically find . -type f -name "*release*.ts" -o -name "*release*.tsx" | while read -r file; do echo "=== $file ===" cat "$file" doneLength of output: 233232
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ComparisonConditionRender.tsx (2)
33-33: LGTM: Import aligns with column-based logic transition.The addition of
ColumnOperatorimport aligns with the PR's objective of transitioning from version-based to column-based logic.
292-292: Verify consistent operator usage across version conditions.The change from
ReleaseOperator.EqualstoColumnOperator.Equalsfor version comparison aligns with the column-based logic transition.Let's verify that this change is consistent across all version-related conditions:
✅ Verification successful
All version conditions consistently use ColumnOperator.Equals
The verification shows that all version-related conditions in the codebase have been updated to use
ColumnOperator.Equals. No instances ofReleaseOperator.Equalswere found withReleaseFilterType.Version, confirming consistent operator usage across version conditions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining ReleaseOperator.Equals usage with version conditions # Expect: No occurrences of ReleaseOperator.Equals with ReleaseFilterType.Version rg -U "type:\s*ReleaseFilterType\.Version[^}]*operator:\s*ReleaseOperator\.Equals" # Also check for the new pattern to ensure consistency rg -U "type:\s*ReleaseFilterType\.Version[^}]*operator:\s*ColumnOperator\.Equals"Length of output: 828
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ComparisonConditionRender.tsx (2)
33-33: LGTM: Import statement is correctly placed and used.The new import for
ColumnOperatoraligns with the codebase's transition to column-based logic.
296-306: Implementation looks good, verify operator consistency.The new Identifier filter implementation is well-structured and follows the existing patterns. The use of
ColumnOperator.Likealigns with the codebase's transition to column-based logic.Let's verify that other components handling identifier conditions consistently use
ColumnOperator.Like:✅ Verification successful
Let me gather more information to verify the operator consistency across the codebase.
This will help us understand:
- How TargetFilterType.Identifier is used in different contexts
- The pattern of ColumnOperator.Like usage across the codebase
- Whether there's consistency in operator usage for identifier conditions
Implementation is correct and consistent with codebase patterns
The verification confirms that the use of
ColumnOperator.Likefor the Identifier filter is correct and consistent with the codebase implementation:
- The database layer (
packages/db/src/schema/target.ts) properly handlesColumnOperator.Likefor identifier conditions through thebuildIdentifierConditionfunction- The validator layer (
packages/validators/src/conditions/index.ts) includesColumnOperator.Likein the schema- Similar column-based filtering patterns using
ColumnOperator.Likeare used in other parts of the codebase (version conditions, column conditions)The implementation aligns with the codebase's column-based filtering approach and maintains consistency across all layers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other identifier condition handling to verify operator consistency rg -l "type: TargetFilterType.Identifier" | xargs rg "operator.*="Length of output: 224
Script:
#!/bin/bash # Search for TargetFilterType.Identifier usage with surrounding context rg -A 5 -B 5 "TargetFilterType\.Identifier" # Also search for any ColumnOperator.Like usage to understand the pattern rg -A 3 -B 3 "ColumnOperator\.Like"Length of output: 8197
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx (1)
346-346: LGTM! Operator change aligns with the codebase modernization.The change from
VersionOperator.EqualstoColumnOperator.Equalsis consistent with the broader transition to column-based operators.Let's verify that this transition is complete across the codebase:
✅ Verification successful
Transition to ColumnOperator.Equals is complete and consistent
The search results confirm that all version-related conditions across the codebase now use
ColumnOperator.Equalsinstead ofVersionOperator. Specifically:
- All version condition implementations in components (
JobComparisonConditionRender.tsx,ComparisonConditionRender.tsx,FlowPolicyNode.tsx) consistently useColumnOperator.Equals- No occurrences of
VersionOperatorwere found in the codebase- The enum definitions show
ColumnOperatoras the standardized operator type for column-based comparisons🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of VersionOperator # Expect: No occurrences of VersionOperator, confirming complete transition # Search for any remaining VersionOperator usage rg "VersionOperator\." # Search for Version-related conditions to ensure they use ColumnOperator ast-grep --pattern 'type: FilterType.Version'Length of output: 72
Script:
#!/bin/bash # Let's try a more comprehensive search approach # Search for any Version-related enums or types rg -i "versionoperator|version.*operator|operator.*version" -A 3 -B 3 # Search for Version filter type usage with more context rg "FilterType\.Version" -A 5 -B 5 # Search for any files containing both Version and Operator references rg -l "Version.*Operator|Operator.*Version" # Look for enum definitions that might contain version operators ast-grep --pattern 'enum $_ { $$$ }'Length of output: 21743
| export type IdentifierOperator = z.infer< | ||
| typeof identifierCondition | ||
| >["operator"]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more concise type definition.
The IdentifierOperator type definition can be simplified.
-export type IdentifierOperator = z.infer<
- typeof identifierCondition
->["operator"];
+export type IdentifierOperator = IdentifierCondition["operator"];Committable suggestion was skipped due to low confidence.
| return ( | ||
| <ColumnConditionRender | ||
| operator={condition.operator} | ||
| value={condition.value} | ||
| setOperator={setOperator} | ||
| setValue={setValue} | ||
| className={className} | ||
| title="Identifier" | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the title configurable.
The "Identifier" title is hardcoded, which might limit component reusability. Consider making it a prop with "Identifier" as the default value.
export const IdentifierConditionRender: React.FC<
- TargetConditionRenderProps<IdentifierCondition>
+ TargetConditionRenderProps<IdentifierCondition> & {
+ title?: string;
+ }
- > = ({ condition, onChange, className }) => {
+ > = ({ condition, onChange, className, title = "Identifier" }) => {
// ... rest of the component
return (
<ColumnConditionRender
// ... other props
- title="Identifier"
+ title={title}
/>
);
};Committable suggestion was skipped due to low confidence.
| const setOperator = (operator: ColumnOperatorType) => | ||
| onChange({ ...condition, operator }); | ||
| const setValue = (value: string) => onChange({ ...condition, value }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing callback functions.
To optimize performance and prevent unnecessary re-renders, consider memoizing the callback functions using useCallback.
- const setOperator = (operator: ColumnOperatorType) =>
- onChange({ ...condition, operator });
- const setValue = (value: string) => onChange({ ...condition, value });
+ const setOperator = useCallback(
+ (operator: ColumnOperatorType) => onChange({ ...condition, operator }),
+ [condition, onChange]
+ );
+ const setValue = useCallback(
+ (value: string) => onChange({ ...condition, value }),
+ [condition, onChange]
+ );Committable suggestion was skipped due to low confidence.
| if (isIdentifierCondition(condition)) | ||
| return ( | ||
| <IdentifierConditionRender | ||
| condition={condition} | ||
| onChange={onChange} | ||
| className={className} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Inconsistent prop passing detected.
The IdentifierConditionRender component is receiving fewer props compared to other condition renderers. While other renderers receive onRemove and depth props, these are omitted here. This inconsistency could lead to unexpected behavior or missing functionality.
Consider applying this change to maintain consistency with other condition renderers:
if (isIdentifierCondition(condition))
return (
<IdentifierConditionRender
condition={condition}
onChange={onChange}
+ onRemove={onRemove}
+ depth={depth}
className={className}
/>
);📝 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 (isIdentifierCondition(condition)) | |
| return ( | |
| <IdentifierConditionRender | |
| condition={condition} | |
| onChange={onChange} | |
| className={className} | |
| /> | |
| ); | |
| if (isIdentifierCondition(condition)) | |
| return ( | |
| <IdentifierConditionRender | |
| condition={condition} | |
| onChange={onChange} | |
| onRemove={onRemove} | |
| depth={depth} | |
| className={className} | |
| /> | |
| ); |
| ColumnOperator, | ||
| DateOperator, | ||
| VersionOperator, | ||
| MetadataOperator, |
There was a problem hiding this comment.
💡 Codebase verification
Transition to enum operators is complete with one exception
The codebase shows consistent usage of ColumnOperator, MetadataOperator, and DateOperator enums across all relevant files, with only one remaining string literal operator comparison in target.ts:
packages/db/src/schema/target.ts:const con = cond.operator === "and" ? and(...subCon)! : or(...subCon)!;
The other string literals found are actually enum definitions themselves, not direct usage in conditions.
🔗 Analysis chain
Verify complete transition from string literals.
Let's ensure all operator string literals have been replaced with enum values across the codebase.
Also applies to: 168-195, 233-236
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining operator string literals that should use enums
echo "=== Searching for potential string literal operators ==="
rg --type typescript 'operator === "(null|regex|like|equals)"' -g '!*.test.ts'
echo -e "\n=== Verifying operator enum usage ==="
rg --type typescript '(Column|Metadata)Operator\.' -g '!*.test.ts'
Length of output: 546
Script:
#!/bin/bash
# Let's try without typescript file type specification
echo "=== Searching for potential string literal operators ==="
rg 'operator\s*===\s*["\'](null|regex|like|equals)["\']' --type-add 'ts:*.ts' -t ts -g '!*.test.ts'
echo -e "\n=== Verifying operator enum usage ==="
rg '(Column|Metadata|Date)Operator\.' --type-add 'ts:*.ts' -t ts -g '!*.test.ts'
# Also search for specific operator strings that might be used in conditions
echo -e "\n=== Checking for raw operator strings ==="
rg '["\'](null|regex|like|equals)["\']' --type-add 'ts:*.ts' -t ts -g '!*.test.ts' -g '!**/migrations/*'
Length of output: 359
Script:
#!/bin/bash
# Search for string literals with simpler patterns
echo "=== Searching for operator string comparisons ==="
rg "operator\s*===\s*['\"]" --type-add 'ts:*.ts' -t ts -g '!*.test.ts'
echo -e "\n=== Verifying enum usage ==="
rg "ColumnOperator\." --type-add 'ts:*.ts' -t ts -g '!*.test.ts'
rg "MetadataOperator\." --type-add 'ts:*.ts' -t ts -g '!*.test.ts'
rg "DateOperator\." --type-add 'ts:*.ts' -t ts -g '!*.test.ts'
echo -e "\n=== Checking operator string usage ==="
rg "['\"]equals['\"]" --type-add 'ts:*.ts' -t ts -g '!*.test.ts' -g '!**/migrations/*'
rg "['\"]like['\"]" --type-add 'ts:*.ts' -t ts -g '!*.test.ts' -g '!**/migrations/*'
rg "['\"]regex['\"]" --type-add 'ts:*.ts' -t ts -g '!*.test.ts' -g '!**/migrations/*'
rg "['\"]null['\"]" --type-add 'ts:*.ts' -t ts -g '!*.test.ts' -g '!**/migrations/*'
Length of output: 13385
| @@ -239,7 +239,7 @@ const StringifiedVersionCondition: React.FC<{ | |||
| <span className="text-muted-foreground"> | |||
| {operatorVerbs[condition.operator]} | |||
| </span> | |||
| <span className="text-white">{condition.value}</span> | |||
| <span className="text-white">{condition.value.replace(/%/g, "")}</span> | |||
There was a problem hiding this comment.
Add null check and document the purpose of removing % signs.
While removing % signs might be intentional, the code could be more robust and clearer:
- Add a null check to prevent runtime errors
- Document why % signs are being removed
Consider this safer implementation:
- <span className="text-white">{condition.value.replace(/%/g, "")}</span>
+ <span className="text-white">
+ {condition.value?.replace(/%/g, "") ?? condition.value}
+ </span>Also, add a comment explaining why % signs are being removed.
📝 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.
| <span className="text-white">{condition.value.replace(/%/g, "")}</span> | |
| <span className="text-white"> | |
| {condition.value?.replace(/%/g, "") ?? condition.value} | |
| </span> |
| const buildIdentifierCondition = (tx: Tx, cond: IdentifierCondition): SQL => { | ||
| if (cond.operator === ColumnOperator.Like) | ||
| return like(target.identifier, cond.value); | ||
| if (cond.operator === ColumnOperator.Equals) | ||
| return eq(target.identifier, cond.value); | ||
| return sql`${target.identifier} ~ ${cond.value}`; | ||
| }; |
There was a problem hiding this comment.
Ensure safe handling of cond.value in regex operation
In the buildIdentifierCondition function, when neither ColumnOperator.Like nor ColumnOperator.Equals matches, the default case returns a regex match using:
return sql`${target.identifier} ~ ${cond.value}`;Ensure that cond.value is properly sanitized or parameterized to prevent potential SQL injection vulnerabilities, especially if it contains user-provided input.
Summary by CodeRabbit
Release Notes
New Features
IdentifierCondition, enhancing the condition management capabilities.ColumnOperatorfor improved condition evaluations across various components.Improvements
ColumnOperatorinstead ofVersionOperator, ensuring consistency in condition handling.Bug Fixes
Chores