fix: Extract reusable filter stuff #173
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request primarily updates the import paths for Changes
Possibly related PRs
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: 16
🧹 Outside diff range and nitpick comments (15)
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/CreatedAtConditionRender.tsx (1)
30-37: Consider internationalizing the "Created at" label.While the implementation is clean and reusable, consider extracting the hardcoded "Created at" string to support internationalization.
+// Import translation utility +import { useTranslation } from 'next-i18next'; export const CreatedAtConditionRender: React.FC< ReleaseConditionRenderProps<CreatedAtCondition> > = ({ condition, onChange, className }) => { + const { t } = useTranslation(); return ( <DateConditionRender setDate={setDate} setOperator={setOperator} value={condition.value} operator={condition.operator} - type="Created at" + type={t('common.createdAt')} className={className} /> ); };packages/validators/package.json (1)
42-45: Consider documenting the new conditions module.Since this is part of a larger refactoring to extract condition-related functionality, it would be helpful to add documentation about the module's purpose and contents.
Consider adding a README.md in the conditions directory explaining:
- The purpose of the conditions module
- Key exports and their usage
- Why it was extracted from targets
apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (1)
34-38: Consider improving responsive design.The current grid layout might not work well on smaller screens. Consider making it more responsive.
- <div className="grid w-full grid-cols-12"> + <div className="grid w-full grid-cols-1 md:grid-cols-12 gap-2"> - <div className="col-span-2 flex items-center rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground"> + <div className="md:col-span-2 flex items-center rounded-md md:rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground"> {capitalCase(type)} </div> - <div className="col-span-10"> + <div className="md:col-span-10">Also applies to: 39-82
apps/webservice/src/app/[workspaceSlug]/_components/filter/DateConditionRender.tsx (1)
17-39: Consider improvements to the date conversion utilityA few suggestions to enhance the
toZonedDateTimefunction:
- The month adjustment (
+ 1) is due to JavaScript's 0-based months. Consider adding a comment to explain this.- The
mspackage is used only once for a simple calculation. Consider using a direct multiplication:offset = -date.getTimezoneOffset() * 60000.const toZonedDateTime = (date: Date): ZonedDateTime => { const timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone; - const offset = -date.getTimezoneOffset() * ms("1m"); + // Convert timezone offset from minutes to milliseconds + const offset = -date.getTimezoneOffset() * 60000; const year = date.getFullYear(); + // JavaScript months are 0-based, so we add 1 const month = date.getMonth() + 1; const day = date.getDate();apps/webservice/src/app/[workspaceSlug]/_components/release-drawer/OverviewContent.tsx (1)
Line range hint
26-31: Consider adding error handling for JSON parsing.The JSON parsing of links metadata could throw an exception if the JSON is malformed. Consider adding try-catch error handling:
const links = metadata[ReservedMetadataKey.Links] != null - ? (JSON.parse(metadata[ReservedMetadataKey.Links]) as Record< - string, - string - >) + ? (() => { + try { + return JSON.parse(metadata[ReservedMetadataKey.Links]) as Record<string, string>; + } catch (e) { + console.error('Failed to parse links metadata:', e); + return null; + } + })() : null;apps/event-worker/src/target-scan/google.ts (1)
Line range hint
108-146: Consider adding type safety for metadata keys.The metadata construction uses string concatenation and direct enum access. Consider creating a type-safe helper function to construct metadata objects, ensuring all keys are valid and preventing typos.
type MetadataValue = string | undefined; type ClusterMetadata = Record<string, MetadataValue>; function createClusterMetadata( cluster: google.container.v1.ICluster, project: string, appUrl: string, versions: { master: SemVer; node: SemVer } ): ClusterMetadata { return { [ReservedMetadataKey.Links]: JSON.stringify({ "Google Console": appUrl }), [ReservedMetadataKey.ExternalId]: cluster.id ?? "", // ... rest of the metadata keys }; }apps/webservice/src/app/[workspaceSlug]/_components/filter/MetadataConditionRender.tsx (3)
30-33: Consider making the condition check more explicit.The condition check could be more explicit to improve readability.
- const setValue = (value: string) => - condition.operator !== MetadataOperator.Null && - onChange({ ...condition, value }); + const setValue = (value: string) => { + if (condition.operator === MetadataOperator.Null) { + return; + } + onChange({ ...condition, value }); + };
62-65: Consider making the popover height responsive.The hardcoded max height might not be optimal for all screen sizes.
- className="scrollbar-thin scrollbar-track-neutral-950 scrollbar-thumb-neutral-800 max-h-[300px] overflow-y-auto p-0 text-sm" + className="scrollbar-thin scrollbar-track-neutral-950 scrollbar-thumb-neutral-800 max-h-[40vh] overflow-y-auto p-0 text-sm"
55-61: Add aria labels for better accessibility.The input fields should have appropriate aria labels for better screen reader support.
<Input placeholder="Key" value={condition.key} onChange={(e) => setKey(e.target.value)} - className="w-full cursor-pointer rounded-l-sm rounded-r-none" + className="w-full cursor-pointer rounded-l-sm rounded-r-none" + aria-label="Metadata key" /> <Input placeholder={ condition.operator === MetadataOperator.Regex ? "^[a-zA-Z]+$" : condition.operator === MetadataOperator.Like ? "%value%" : "Value" } value={condition.value} onChange={(e) => setValue(e.target.value)} - className="rounded-l-none rounded-r-sm hover:bg-neutral-800/50" + className="rounded-l-none rounded-r-sm hover:bg-neutral-800/50" + aria-label="Metadata value" />Also applies to: 112-124
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx (1)
Line range hint
171-186: Consider using a condition type map for better maintainability.The current implementation uses multiple if statements to determine which component to render. As more condition types are added, this could become harder to maintain. Consider using a mapping approach:
const conditionComponents = { comparison: StringifiedComparisonCondition, metadata: StringifiedMetadataCondition, createdAt: StringifiedCreatedAtCondition, version: StringifiedVersionCondition, } as const; const StringifiedReleaseCondition: React.FC<{ condition: ReleaseCondition; depth?: number; truncate?: boolean; tabbed?: boolean; }> = ({ condition, depth = 0, truncate = false, tabbed = false }) => { const Component = getConditionComponent(condition, conditionComponents); return Component ? <Component condition={condition} depth={depth} truncate={truncate} tabbed={tabbed} /> : null; };This approach would:
- Make it easier to add new condition types
- Reduce cognitive complexity
- Make the code more maintainable
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/OverviewContent.tsx (1)
15-15: Consider future metadata handling evolution.The move of
ReservedMetadataKeyto the conditions package suggests a shift towards more generalized metadata handling. As this evolves, consider:
- Whether the current metadata rendering in
TargetMetadataInfomight need to be more dynamic- If the hard-coded metadata key checks should be replaced with a more configurable approach
- Whether metadata validation logic should also be centralized in the conditions package
packages/db/src/schema/release.ts (4)
Line range hint
23-23: Consider adding an index toreleaseFilterfor better query performanceSince
releaseFilteris used in querying release dependencies, adding a GIN or BTREE index on thisjsonbfield could significantly improve query performance when filtering releases based on conditions.
Line range hint
73-93: Sanitize regex patterns to prevent SQL injection inbuildMetadataConditionIn
buildMetadataCondition, thecond.valuefor theregexoperator is interpolated directly into the SQL query usingsql\${releaseMetadata.value} ~ ${cond.value}`. This could lead to SQL injection ifcond.value` contains malicious input. Ensure that the regex patterns are properly sanitized or use parameterized queries to prevent injection attacks.Apply this diff to use parameterized queries:
- sql`${releaseMetadata.value} ~ ${cond.value}` + sql`${releaseMetadata.value} ~ ${sql.literal(cond.value)}`
Line range hint
85-92: Add validation for date parsing inbuildCreatedAtConditionWhen converting
cond.valueto aDateobject, invalid date strings could result inInvalid Dateobjects, leading to unexpected behavior in queries. Consider adding validation to ensure thatcond.valueis a valid date format before using it in the SQL condition.Apply this diff to add date validation:
const buildCreatedAtCondition = (cond: CreatedAtCondition): SQL => { + const date = new Date(cond.value); + if (isNaN(date.getTime())) { + throw new Error(`Invalid date format: ${cond.value}`); + } - const date = new Date(cond.value); if (cond.operator === ReleaseOperator.Before) return lt(release.createdAt, date); // ... rest of the function };
Line range hint
95-101: Explicitly handle unsupported operators inbuildVersionConditionCurrently, if
cond.operatoris neitherEqualsnorLike, the function defaults to performing a regex match. This may lead to unintended behavior if an unsupported operator is provided. Consider explicitly handling all expected operators and throwing an error for any unsupported ones.Apply this diff to handle unsupported operators safely:
const buildVersionCondition = (cond: VersionCondition): SQL => { if (cond.operator === ReleaseOperator.Equals) return eq(release.version, cond.value); if (cond.operator === ReleaseOperator.Like) return like(release.version, cond.value); + if (cond.operator === ReleaseOperator.Regex) + return sql`${release.version} ~ ${sql.literal(cond.value)}`; + throw new Error(`Unsupported operator: ${cond.operator}`); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (37)
- apps/event-worker/src/target-scan/gke.ts (1 hunks)
- apps/event-worker/src/target-scan/google.ts (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/DateConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/MetadataConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobMetadata.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobProperties.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/CreatedAtConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/MetadataConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-drawer/OverviewContent.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-drawer/ReleaseDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/KindConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/MetadataConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ProviderConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionBadge.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/OverviewContent.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (1 hunks)
- integrations/google-compute-scanner/src/gke.ts (1 hunks)
- packages/db/src/schema/release.ts (1 hunks)
- packages/db/src/schema/target.ts (1 hunks)
- packages/validators/package.json (1 hunks)
- packages/validators/src/conditions/date-condition.ts (1 hunks)
- packages/validators/src/conditions/index.ts (1 hunks)
- packages/validators/src/conditions/metadata-condition.ts (1 hunks)
- packages/validators/src/releases/conditions/comparison-condition.ts (1 hunks)
- packages/validators/src/releases/conditions/created-at-condition.ts (0 hunks)
- packages/validators/src/releases/conditions/index.ts (0 hunks)
- packages/validators/src/releases/conditions/release-condition.ts (1 hunks)
- packages/validators/src/targets/conditions/comparison-condition.ts (1 hunks)
- packages/validators/src/targets/conditions/index.ts (0 hunks)
- packages/validators/src/targets/conditions/metadata-condition.ts (0 hunks)
- packages/validators/src/targets/conditions/target-condition.ts (1 hunks)
💤 Files with no reviewable changes (4)
- packages/validators/src/releases/conditions/created-at-condition.ts
- packages/validators/src/releases/conditions/index.ts
- packages/validators/src/targets/conditions/index.ts
- packages/validators/src/targets/conditions/metadata-condition.ts
✅ Files skipped from review due to trivial changes (3)
- integrations/google-compute-scanner/src/gke.ts
- packages/db/src/schema/target.ts
- packages/validators/src/conditions/index.ts
🧰 Additional context used
🪛 Biome
packages/validators/src/conditions/date-condition.ts
[error] 10-10: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (50)
packages/validators/src/conditions/date-condition.ts (3)
1-8: LGTM! Well-defined operator union type.The operator union type effectively covers all necessary date comparison scenarios.
14-22: LGTM! Well-structured type definitions.The type literals and enum definitions are clear and maintain type safety by matching the operator union type.
17-22: Verify usage of exported types in dependent components.Let's ensure the exported
DateOperatorenum and condition types are being used consistently across the codebase.Also applies to: 30-30, 38-38
✅ Verification successful
DateOperator enum is used consistently across the codebase
The verification shows that the
DateOperatorenum andCreatedAtConditiontype are being used correctly and consistently:
- The enum values (Before, After, BeforeOrOn, AfterOrOn) are properly referenced in components like
CreatedAtConditionRender.tsx- Type imports and usage are consistent across multiple components:
DateConditionRender.tsxCreatedAtConditionRender.tsxReleaseConditionRender.tsxReleaseConditionBadge.tsx- The types are also properly used in the database schema (
packages/db/src/schema/release.ts) and validator files🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of DateOperator and condition types # Check DateOperator enum usage echo "Checking DateOperator usage:" rg -A 2 "DateOperator\." --type typescript # Check condition type imports echo -e "\nChecking condition type imports:" rg -A 2 "import.*(?:CreatedAtCondition|UpdatedAtCondition).*from.*date-condition" --type typescriptLength of output: 364
Script:
#!/bin/bash # Let's try with different file extensions and without type specification # Check DateOperator enum usage echo "Checking DateOperator usage:" rg -A 2 "DateOperator\." --type ts --type tsx # Check condition type imports echo -e "\nChecking condition type imports:" rg -A 2 "from ['\"].*date-condition" --type ts --type tsx # Find all files that might be using these types echo -e "\nFinding potential dependent files:" fd -e ts -e tsx | xargs rg "DateOperator|CreatedAtCondition|UpdatedAtCondition"Length of output: 4530
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/MetadataConditionRender.tsx (4)
1-7: LGTM! Good job on code reuse.The import changes align well with the PR objective of extracting reusable filter functionality. Moving the
MetadataConditiontype to a more appropriate location and reusing theMetadataConditionRendercomponent promotes better code organization and reduces duplication.
8-10: LGTM! Clear and descriptive component naming.The rename to
TargetMetadataConditionRenderbetter describes the component's specific purpose in the target context, improving code clarity.
19-24: LGTM! Clean and efficient implementation.The component efficiently delegates rendering to the reused
MetadataConditionRendercomponent, passing all necessary props with proper null handling for metadata keys.
11-17: Verify error and loading state handling in the parent component.The component makes two dependent queries but doesn't explicitly handle loading or error states. While the
enabledcondition prevents the metadata keys query when workspace data is unavailable, we should verify that the parent component or the reusedMetadataConditionRenderproperly handles these states.Let's check how these states are handled:
✅ Verification successful
Loading and error states are properly handled through the component hierarchy
After examining the components, I can confirm that:
- The base
MetadataConditionRendercomponent in the filter directory is a presentational component that safely handles undefined metadata keys by providing a fallback empty array:metadataKeys={metadataKeys.data ?? []}- Both wrapper components (Target and Release) properly gate their dependent queries using the
enabledoption- The Release component has additional safeguards with null coalescing:
systemMetadataKeys.data ?? workspaceMetadataKeys.data ?? []The loading and error states are implicitly handled by:
- Preventing dependent queries until parent data is available
- Providing safe fallbacks for undefined data
- Using TypeScript's strict null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how loading and error states are handled in the MetadataConditionRender component # Look for loading/error handling patterns in the reused component ast-grep --pattern 'if ($loading || $error) { $$$ }' # Check for isLoading/isError usage rg -A 3 'isLoading|isError' '../filter/MetadataConditionRender'Length of output: 194
Script:
#!/bin/bash # Let's try to find the component and its usages with more accurate paths fd MetadataConditionRender # Once we find the file, let's check its content for loading/error handling rg -A 3 "isLoading|isError|isSuccess" --type tsx # Look for any conditional rendering patterns that might handle these states ast-grep --pattern 'if ($cond) { return $$$ }'Length of output: 440
Script:
#!/bin/bash # Let's examine the content of all MetadataConditionRender components cat apps/webservice/src/app/[workspaceSlug]/_components/target-condition/MetadataConditionRender.tsx echo "---" cat apps/webservice/src/app/[workspaceSlug]/_components/release-condition/MetadataConditionRender.tsx echo "---" cat apps/webservice/src/app/[workspaceSlug]/_components/filter/MetadataConditionRender.tsx # Also check for loading states with ripgrep (without file type restriction) rg -A 3 'isLoading|isError|isSuccess' apps/webservice/src/app/[workspaceSlug]/_components/target-condition/MetadataConditionRender.tsxLength of output: 7623
packages/validators/src/releases/conditions/comparison-condition.ts (2)
Line range hint
10-38: LGTM! Well-structured schema and type definitions.The schema and type definitions provide strong type safety and runtime validation while maintaining composability through the union of different condition types.
3-4: LGTM! Import restructuring aligns with code organization goals.The import path changes reflect a better organization of condition-related code by moving common conditions to a shared location. This supports the PR's objective of extracting reusable filter functionality.
Let's verify the consistency of these imports:
Also applies to: 6-7
✅ Verification successful
Import paths are correctly structured and consistent
The verification confirms that:
- The imported files exist at the correct locations:
packages/validators/src/conditions/date-condition.tsandpackages/validators/src/conditions/metadata-condition.ts- The import paths are consistent across the codebase, with other files like
release-condition.tsusing the same import structure- The imports align with the actual file locations in the conditions directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and consistency of the imported files # Test 1: Check if the imported condition files exist fd --type f "date-condition.js|metadata-condition.js|index.js" --exec echo "Found: {}" # Test 2: Check for any remaining old imports that might have been missed rg --type ts "from ['\"].*/(created-at-condition|metadata-condition)['\"]"Length of output: 244
Script:
#!/bin/bash # Let's try a more targeted search for the TypeScript source files # Check if the conditions directory exists and list its contents fd --type f --extension ts "." packages/validators/src/conditions # Search for any imports of these conditions across the codebase rg --type ts "from.*conditions/(date-condition|metadata-condition)" # Check if there are any .ts files that might not have been compiled to .js yet fd --type f "date-condition.ts|metadata-condition.ts" packages/validators/src/conditionsLength of output: 1207
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/CreatedAtConditionRender.tsx (2)
1-8: LGTM! Well-organized imports.The imports are logically structured and the migration to use
DateOperatorfrom the conditions package aligns with the PR's objective of extracting reusable filter components.
Line range hint
13-28: LGTM! Well-implemented event handlers.The handlers are well-typed and handle dates correctly with proper timezone consideration. The use of specific
DateOperatorenum values provides good type safety.apps/webservice/src/app/[workspaceSlug]/_components/target-condition/KindConditionRender.tsx (2)
6-6: LGTM: Clean import of the new componentThe import statement correctly references the new ChoiceConditionRender component from the filter directory.
26-32: Verify the integration with ChoiceConditionRenderThe implementation looks correct, but let's ensure all required props are properly typed and passed.
Let's verify the component's interface:
✅ Verification successful
Props interface matches the usage in KindConditionRender
The verification confirms that all required props are properly typed and passed:
type: string ✓ (passed as "kind")onSelect: (value: string) => void ✓ (passed as setKind)selected: string | null ✓ (passed as condition.value)options: { key: string; value: string; display: string }[] ✓ (passed as options)className: string | undefined ✓ (passed as className)The implementation is consistent with other usages in the codebase (e.g., ProviderConditionRender).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ChoiceConditionRender props interface matches usage # Search for ChoiceConditionRender type definition ast-grep --pattern 'type ChoiceConditionRenderProps = { $$$ }' # Search for other usages of ChoiceConditionRender for consistency rg -A 5 "ChoiceConditionRender"Length of output: 5459
packages/validators/src/targets/conditions/comparison-condition.ts (1)
Line range hint
12-35: LGTM! The schema and type definitions are well-structured.The ComparisonCondition schema and type definitions maintain a clean and type-safe structure, properly integrating with the imported conditions.
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ProviderConditionRender.tsx (1)
6-6: LGTM! Import of reusable component.The import of
ChoiceConditionRenderaligns with the PR's objective of extracting reusable filter functionality.packages/validators/src/conditions/metadata-condition.ts (2)
48-53: LGTM! Good addition of MetadataOperator enum.The enum provides a centralized, type-safe way to reference metadata operators that were previously defined as string literals across different schemas.
48-53: 🛠️ Refactor suggestionConsider using MetadataOperator enum in Zod schemas.
The schemas currently use string literals for operators. Consider refactoring them to use the new enum for better type safety and maintainability.
Example refactor:
export const nullCondition = z.object({ type: z.literal("metadata"), key: z.string().min(1), value: z.string().optional(), - operator: z.literal("null"), + operator: z.literal(MetadataOperator.Null), }); export const equalsCondition = z.object({ type: z.literal("metadata"), key: z.string().min(1), value: z.string().min(1), - operator: z.literal("equals").optional(), + operator: z.literal(MetadataOperator.Equals).optional(), });Note: This would be a breaking change if these schemas are used elsewhere in the codebase.
Let's check the usage of these schemas:
packages/validators/package.json (1)
42-45: Verify the existence of condition module files.The export configuration follows the established pattern and properly declares both TypeScript and JavaScript paths.
Let's verify the existence of the referenced files:
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionRender.tsx (2)
14-14: LGTM! Import change improves component specificity.The rename from
MetadataConditionRendertoReleaseMetadataConditionRenderbetter reflects the component's specific responsibility in handling release metadata conditions.
Line range hint
37-43: LGTM! Component usage aligns with the broader refactoring.The update to use
ReleaseMetadataConditionRenderis consistent with the architectural changes to improve modularity in metadata condition handling.Let's verify the refactoring structure:
✅ Verification successful
The component usage is correctly aligned with the modular architecture
The verification confirms that:
- The
ReleaseMetadataConditionRenderis properly modularized in its own file and is only imported where needed- There's a clear separation between different types of metadata condition renderers (Release, Target, Filter) with their specific implementations
- The component hierarchy follows the intended architecture with specialized components for different contexts (release, target, filter)
- The usage in
ReleaseConditionRender.tsxis consistent with this modular structureThe refactoring maintains clean separation of concerns and proper component organization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the separation of generic and release-specific metadata condition handling # Test 1: Check for the existence of both generic and release-specific components echo "Checking for metadata condition components..." rg -l "MetadataConditionRender|ReleaseMetadataConditionRender" --type tsx # Test 2: Verify the component hierarchy and dependencies echo "Checking component dependencies..." ast-grep --pattern 'import { $_ } from "./MetadataConditionRender"'Length of output: 702
Script:
#!/bin/bash # Let's try with a different file extension and check component structure echo "Checking for metadata condition components..." rg -l "MetadataConditionRender|ReleaseMetadataConditionRender" -g "*.ts*" # Check the implementation of MetadataConditionRender echo -e "\nChecking MetadataConditionRender implementation..." rg -A 10 "export.*MetadataConditionRender" -g "*.ts*" # Check if there are any other components importing ReleaseMetadataConditionRender echo -e "\nChecking ReleaseMetadataConditionRender usage..." rg "import.*ReleaseMetadataConditionRender" -g "*.ts*"Length of output: 5512
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobMetadata.tsx (2)
Line range hint
1-57: Implementation looks solid!The component is well-structured with proper TypeScript types, good accessibility practices, and efficient metadata handling.
4-4: LGTM! Import path update is consistent with the refactoring.The change to import
ReservedMetadataKeyfrom@ctrlplane/validators/conditionsaligns with the broader refactoring effort to reorganize metadata-related code.Let's verify this refactoring is consistent across the codebase:
✅ Verification successful
Import path refactoring is complete and consistent
The verification confirms that:
- No imports remain from the old path
@ctrlplane/validators/targets- All 11 files consistently use the new import path
@ctrlplane/validators/conditions- The refactoring has been thoroughly applied across different parts of the codebase including:
- Web service components
- Event worker
- Google Compute Scanner integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ReservedMetadataKey is no longer imported from targets # and all imports now use the new path # Check for any remaining imports from the old path echo "Checking for old imports from targets..." rg "ReservedMetadataKey.*@ctrlplane/validators/targets" # Verify new imports are used consistently echo "Checking usage of new import path..." rg "ReservedMetadataKey.*@ctrlplane/validators/conditions"Length of output: 1928
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionRender.tsx (2)
15-15: LGTM! Import statement updated for better specificity.The renamed import better reflects the component's specific use in the target context.
Line range hint
39-45: Verify consistent component renaming across the codebase.The component renaming from
MetadataConditionRendertoTargetMetadataConditionRenderlooks good, but let's ensure this change is consistent across all usage points.apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (1)
1-14: LGTM! Well-organized imports.The imports are properly organized and all dependencies are being used effectively in the component.
packages/validators/src/releases/conditions/release-condition.ts (1)
3-4: Architectural improvement: Good consolidation of condition types.The movement of
CreatedAtConditionandMetadataConditionto higher-level modules improves reusability and promotes better separation of concerns. However, consider importingMetadataConditiondirectly from its source file instead of the barrel file (index.js) for better maintainability.Let's verify that other files are consistently using these new paths:
Also applies to: 7-8
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobProperties.tsx (1)
5-5: LGTM! Verify enum consistency.The import change aligns with the broader refactoring effort to better organize validators. The change is consistent with similar updates across other components.
Let's verify the enum consistency between the old and new locations:
✅ Verification successful
Let me search for any references to the old location to ensure completeness of the refactoring.
Import change is safe and consistent
The verification confirms:
- ReservedMetadataKey is properly defined in the new location (
packages/validators/src/conditions/metadata-condition.ts)- The enum includes all necessary keys including
Linkswhich is used in this component- No references to the old location were found, indicating a complete refactor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ReservedMetadataKey maintains consistent values # after the move from targets to conditions # Check both old and new locations for the enum definition echo "Checking ReservedMetadataKey definitions:" rg -A 5 "enum ReservedMetadataKey"Length of output: 725
Script:
#!/bin/bash # Search for any remaining references to ReservedMetadataKey in the targets directory # and any imports from the old location rg -g '!*.{json,md}' "validators/targets.*ReservedMetadataKey"Length of output: 64
packages/validators/src/targets/conditions/target-condition.ts (2)
Line range hint
14-93: LGTM! No functional changes in the implementation.The core functionality remains unchanged, maintaining all the type definitions, constants, and utility functions while properly preserving the references to the moved metadata condition types.
3-3: Verify the refactored import paths.The imports for
MetadataConditionandmetadataConditionhave been moved from local to parent directory, which aligns with the PR objective of extracting reusable filter components.Let's verify the new import structure:
Also applies to: 8-8
✅ Verification successful
Import refactoring is correctly implemented
The verification confirms that:
MetadataConditionis properly exported frompackages/validators/src/conditions/metadata-condition.ts- It's correctly re-exported through
packages/validators/src/conditions/index.tsviaexport * from "./metadata-condition.js"- The new import path
../../conditions/index.jsintarget-condition.tscorrectly references these exportsThe import refactoring aligns with the PR's objective of extracting reusable filter components while maintaining proper module organization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and exports of the new metadata condition location # Test 1: Check if the new file exists and exports the required symbols ast-grep --pattern 'export { $$$metadataCondition$$$ }' conditions/index.js ast-grep --pattern 'export type { $$$MetadataCondition$$$ }' conditions/index.js # Test 2: Check for any remaining references to the old location rg -l "metadata-condition\.js" --type tsLength of output: 449
Script:
#!/bin/bash # Let's check the actual exports in the conditions/index.ts file rg -A 5 "export.*MetadataCondition" packages/validators/src/conditions/index.ts # Check if there are any other files exporting MetadataCondition ast-grep --pattern 'export $$$MetadataCondition$$$' # Check the actual path structure fd metadata-conditionLength of output: 216
Script:
#!/bin/bash # Let's check the content of the metadata-condition.ts and index.ts files rg -A 5 "export.*MetadataCondition" packages/validators/src/conditions/metadata-condition.ts # Check if index.ts re-exports from metadata-condition rg "export.*from.*metadata-condition" packages/validators/src/conditions/index.ts # Check the actual imports in the target file cat packages/validators/src/targets/conditions/target-condition.tsLength of output: 3907
apps/webservice/src/app/[workspaceSlug]/_components/release-drawer/ReleaseDrawer.tsx (1)
14-14: LGTM! Import source change aligns with the broader refactoring.The change is part of a consistent pattern across the codebase to centralize condition-related code.
Let's verify the consistency of this import change across the codebase:
✅ Verification successful
Import change for ReservedMetadataKey is consistently applied across the codebase
The verification confirms that:
- No instances of old imports from
@ctrlplane/validators/(releases|targets)were found- All 11 files consistently import
ReservedMetadataKeyfrom@ctrlplane/validators/conditions🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ReservedMetadataKey is consistently imported from @ctrlplane/validators/conditions # Test 1: Check for any remaining imports from the old path echo "Checking for old imports..." rg "ReservedMetadataKey.*@ctrlplane/validators/(releases|targets)" # Test 2: Verify new imports echo "Verifying new imports..." rg "ReservedMetadataKey.*@ctrlplane/validators/conditions"Length of output: 1889
apps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsx (2)
14-14: LGTM! Import path update aligns with codebase refactor.The change is part of a coordinated effort to standardize ReservedMetadataKey imports across the codebase.
Let's verify the consistency of this change:
✅ Verification successful
Import path change is consistent across the codebase
The verification confirms:
- No remaining imports from the old path (
validators/targets)- All imports consistently use the new path (
validators/conditions)- The enum is properly defined in
packages/validators/src/conditions/metadata-condition.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ReservedMetadataKey is consistently imported from conditions # and that the enum values match between the old and new locations # Test 1: Check if there are any remaining imports from the old path rg "ReservedMetadataKey.*validators/targets" # Test 2: Verify the new import path is used consistently rg "ReservedMetadataKey.*validators/conditions" # Test 3: Check both files to ensure enum values match ast-grep --pattern 'enum ReservedMetadataKey { $$$ }'Length of output: 2466
Line range hint
37-39: Verify enum value compatibility.The code uses
ReservedMetadataKey.Linksfor metadata filtering. Since the import source has changed, we should ensure this enum value exists in the new location with the same definition.✅ Verification successful
The
Linksenum value exists and is correctly defined in the new locationFrom the search results, I can confirm that
ReservedMetadataKeyenum is defined in@ctrlplane/validators/conditionswith theLinksvalue properly defined as"ctrlplane/links". This is evident from the following definition found inpackages/validators/src/conditions/metadata-condition.ts:export enum ReservedMetadataKey { ExternalId = "ctrlplane/external-id", Links = "ctrlplane/links", ParentTargetIdentifier = "ctrlplane/parent-target-identifier", ... }The enum is being used consistently across the codebase for metadata filtering, including in the file under review. The value and its usage remain compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Links enum value exists in the new location # Search for Links in the new ReservedMetadataKey definition ast-grep --pattern 'enum ReservedMetadataKey { $$$ Links = $$$ $$$ }'Length of output: 77
Script:
#!/bin/bash # Let's try a broader search to find any enum definitions containing Links ast-grep --pattern 'enum $_ { $$$ Links = $_ $$$ }' # Also search for any occurrence of ReservedMetadataKey rg "ReservedMetadataKey" -A 5 -B 5 # Search for files containing "Links" as an enum value rg "Links\s*=\s*" --type typescriptLength of output: 34579
apps/event-worker/src/target-scan/gke.ts (1)
11-11: Verify enum consistency after import path change.The import path change for
ReservedMetadataKeyaligns with the PR objective of extracting reusable filter functionality. However, we should verify that the enum values remain consistent between the old and new locations to prevent any breaking changes.Let's verify the enum values in both locations:
✅ Verification successful
Import path change for ReservedMetadataKey is valid and safe
The verification confirms that
ReservedMetadataKeyis properly defined inpackages/validators/src/conditions/metadata-condition.tswith all the required enum values that are being used across the codebase:
ExternalIdLinksParentTargetIdentifierKubernetesVersionKubernetesFlavorThe usage patterns in the codebase align perfectly with these enum values, particularly in the GKE-related files where they're being used for metadata assignment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare ReservedMetadataKey enum values between old and new locations # Expected: Enum values should be identical to prevent breaking changes echo "Checking ReservedMetadataKey definitions:" # Search in both old and new locations rg -A 10 "ReservedMetadataKey\s*=\s*" --type typescript # Check usage patterns to ensure compatibility echo -e "\nChecking usage patterns:" rg "ReservedMetadataKey\." --type typescriptLength of output: 332
Script:
#!/bin/bash # Let's try without typescript file type specification echo "Checking ReservedMetadataKey definitions:" rg -A 10 "ReservedMetadataKey\s*=\s*" echo -e "\nChecking usage patterns:" rg "ReservedMetadataKey\." # Also search for the file paths to understand the structure echo -e "\nLocating relevant files:" fd "validators" --type fLength of output: 3082
Script:
#!/bin/bash # Let's find and examine the enum definition echo "Searching for ReservedMetadataKey enum definition:" rg -B 2 -A 10 "enum\s+ReservedMetadataKey" # Also check the specific files in the validators directory echo -e "\nChecking validators directory content:" fd -e ts "conditions\.ts" -x cat {}Length of output: 1060
apps/webservice/src/app/[workspaceSlug]/_components/release-drawer/OverviewContent.tsx (1)
9-9: Verify the impact of the import path change.The import path change from
@ctrlplane/validators/targetsto@ctrlplane/validators/conditionsappears to be part of a larger refactoring effort. The component's functionality remains intact as it correctly usesReservedMetadataKeyfor both link parsing and metadata display.Let's verify that this change is consistent across the codebase and that the new path is valid:
✅ Verification successful
Let me generate additional verification scripts to check the new location of
ReservedMetadataKey:
Import path change is correctly implemented and consistent
The verification confirms that:
ReservedMetadataKeyis now properly defined in@ctrlplane/validators/conditions/metadata-condition.ts- All imports across the codebase consistently use the new path
@ctrlplane/validators/conditions- No remaining imports from the old path
@ctrlplane/validators/targetsexist- The usage patterns remain consistent with the enum's definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of ReservedMetadataKey imports and its usage # Test 1: Check if there are any remaining imports from the old path echo "Checking for any remaining old imports..." rg "ReservedMetadataKey.*@ctrlplane/validators/targets" # Test 2: Verify the new import path exists and contains the enum echo "Verifying the new import path..." fd -e ts -e tsx "conditions\.tsx?$" -d 3 "validators" # Test 3: Check the usage pattern of ReservedMetadataKey to ensure consistency echo "Checking usage patterns..." rg "ReservedMetadataKey\." -A 2Length of output: 5738
Script:
#!/bin/bash # Check the actual definition and location of ReservedMetadataKey in the new path echo "Checking ReservedMetadataKey definition in conditions..." rg -g "*.ts" -g "*.tsx" "export.*enum.*ReservedMetadataKey" -A 5 # Check all imports of ReservedMetadataKey to ensure consistency echo "Checking all imports of ReservedMetadataKey..." rg "import.*ReservedMetadataKey" -A 1Length of output: 3582
apps/event-worker/src/target-scan/google.ts (1)
10-10: Verify the import compatibility.The import change aligns with the PR objective of centralizing filter-related code. However, let's verify that the enum exists in the new location with the same values.
✅ Verification successful
Import change is compatible and correctly implemented
The verification confirms that:
- The enum exists in the new location
@ctrlplane/validators/conditions- All enum values used in
google.ts(Links,ExternalId,KubernetesFlavor,KubernetesVersion) are present in the new location- No other files are still using the old import path from
@ctrlplane/validators/targets🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ReservedMetadataKey exists in both locations and compare their values # Test 1: Check if the enum exists in both locations echo "Checking enum existence in both locations..." rg --type-add 'ts:*.{ts,tsx}' -t ts 'export .*enum.*ReservedMetadataKey' -g '!node_modules' # Test 2: Compare the enum values used in this file with the available ones echo "Checking enum values used in this file..." rg --type-add 'ts:*.{ts,tsx}' -t ts 'ReservedMetadataKey\.' apps/event-worker/src/target-scan/google.tsLength of output: 721
Script:
#!/bin/bash # Let's check the enum values in the new location to ensure compatibility echo "Checking enum values in the new location..." rg --type-add 'ts:*.{ts,tsx}' -t ts -A 10 'export enum ReservedMetadataKey' packages/validators/src/conditions/metadata-condition.ts # Let's also check if there are any other files using this enum from the old location echo "Checking for other files using the old import path..." rg --type-add 'ts:*.{ts,tsx}' -t ts '@ctrlplane/validators/targets.*ReservedMetadataKey' -g '!node_modules'Length of output: 714
apps/webservice/src/app/[workspaceSlug]/_components/filter/MetadataConditionRender.tsx (2)
1-24: LGTM! Well-organized imports and clear type definitions.The imports are logically grouped and the
MetadataConditionRenderPropstype is well-defined with appropriate types for each prop.
25-131: Overall implementation is solid and well-structured.The component demonstrates good practices in:
- Responsive layout using grid system
- Clear separation of concerns
- Proper state management
- Conditional rendering
- User-friendly interface with appropriate feedback
The suggested improvements are minor enhancements to an already solid implementation.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (1)
13-13: LGTM! Import path update aligns with the refactoring objectives.The change to import
ReservedMetadataKeyfrom@ctrlplane/validators/conditionsinstead of@ctrlplane/validators/targetsis consistent with the PR's goal of extracting reusable components.Let's verify that this import path change is consistent across the codebase:
✅ Verification successful
Import path migration is complete and consistent
The verification confirms that:
- No instances of the old import path (
@ctrlplane/validators/targets) remain in the codebase- The new import path (
@ctrlplane/validators/conditions) is consistently used across 11 files in the codebase🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ReservedMetadataKey is consistently imported from the new path # and no old imports remain # Test 1: Check for any remaining imports from the old path echo "Checking for old imports..." rg "ReservedMetadataKey.*@ctrlplane/validators/targets" # Test 2: Verify new imports are used consistently echo "Checking new imports..." rg "ReservedMetadataKey.*@ctrlplane/validators/conditions"Length of output: 1876
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx (2)
4-8: 🛠️ Refactor suggestionConsider consolidating condition type imports.
The import of
MetadataConditionandCreatedAtConditionhas been moved to@ctrlplane/validators/conditions, butComparisonConditionis still imported from@ctrlplane/validators/releases. According to the PR objectives of extracting reusable filter components, consider moving all condition-related types to the conditions module for better organization.Let's verify the current location of condition types:
#!/bin/bash # Description: Check the current location and usage of condition types # Expected: All condition types should be in the conditions module # Test 1: Check if ComparisonCondition is defined in both locations echo "Checking ComparisonCondition definition locations:" rg -l "export.*ComparisonCondition" --type typescript # Test 2: Check other files that might still use the old import path echo "Checking files still using old import paths:" rg -l "from.*@ctrlplane/validators/releases.*ComparisonCondition" --type typescript
Line range hint
22-27: Consider moving type guards with their corresponding types.The type guards (
isComparisonCondition,isCreatedAtCondition, etc.) are imported from@ctrlplane/validators/releaseswhile some of their corresponding types are now in@ctrlplane/validators/conditions. Consider moving the type guards alongside their types to maintain better code organization and type safety.Let's verify the current structure:
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/TargetConditionBadge.tsx (3)
Line range hint
141-207: Well-structured implementation with good UX considerations.The components are well-organized with:
- Clear separation of concerns for different condition types
- Consistent styling and component patterns
- Good UX with hover cards for detailed views
- Proper type safety with type guards
Line range hint
1-8: Consider consolidating metadata-related types in the conditions module.While
MetadataConditionhas been moved to@ctrlplane/validators/conditions, other related types likeComparisonConditionremain in the targets module. Consider moving all metadata-related types to the conditions module for better organization and consistency.Let's check for other metadata-related types:
Line range hint
141-152: Verify if operator should be optional in MetadataCondition.The component includes a fallback for undefined operator:
condition.operator ?? "equals". This suggests thatoperatoris optional in theMetadataConditiontype. Please verify if this is intentional or if we should enforce operator as a required field for type safety.Let's check the type definition:
✅ Verification successful
The operator fallback is correct and intentional
The
operatorfield is explicitly marked as optional only in theequalsConditionschema (operator: z.literal("equals").optional()), while it's required in other condition types. This is a deliberate design choice where:
- When operator is undefined, it defaults to "equals" as the most common case
- Other operators (like, regex, null) must be explicitly specified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MetadataCondition type definition ast-grep --pattern 'export type MetadataCondition = { $$$ }'Length of output: 64
Script:
#!/bin/bash # Let's try a broader search for type definitions related to conditions rg -l "type.*Condition" --type typescript # Then let's examine the content of these files echo "=== Files containing condition type definitions ===" rg "type.*Condition.*=.*{" --type typescript -A 5Length of output: 349
Script:
#!/bin/bash # Let's try a simpler search without file type filtering # First find files containing "Condition" type definitions rg -l "type.*Condition" # Then search for the specific content echo "=== Content of files with Condition types ===" rg "type\s+\w*Condition.*=" -A 5Length of output: 14574
Script:
#!/bin/bash # Let's examine the specific MetadataCondition type and its usage echo "=== MetadataCondition definition ===" rg "export.*type.*MetadataCondition.*=" -A 10 packages/validators/src/conditions/metadata-condition.ts echo "=== MetadataCondition schema validation ===" rg "export const metadataCondition" -A 10 packages/validators/src/conditions/metadata-condition.tsLength of output: 874
Script:
#!/bin/bash # Let's examine the individual condition types that make up the union echo "=== Individual condition schemas ===" rg "export const (equals|like|regex|null)Condition = " -A 5 packages/validators/src/conditions/metadata-condition.tsLength of output: 871
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetDrawer.tsx (1)
19-19: LGTM! Import path update aligns with the refactoring.The change is part of a broader effort to extract and consolidate shared code, moving
ReservedMetadataKeyfrom the targets package to the conditions package.Let's verify that this refactoring is consistent across the codebase:
✅ Verification successful
Import path change is consistently applied across the codebase
The verification confirms that:
- No references to the old import path (
@ctrlplane/validators/targets) exist in the codebase- The new import path (
@ctrlplane/validators/conditions) is consistently used across 11 files- The change aligns with the refactoring effort to extract and consolidate shared code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no references to the old import path exist # and all components use the new path consistently # Test 1: Check for any remaining imports from the old path echo "Checking for old import path usage..." rg "ReservedMetadataKey.*@ctrlplane/validators/targets" # Test 2: Verify consistent usage of new import path echo "Checking new import path usage..." rg "ReservedMetadataKey.*@ctrlplane/validators/conditions"Length of output: 1916
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/MetadataConditionRender.tsx (3)
1-1: Import path updated correctlyThe import statement for
MetadataConditionhas been updated to@ctrlplane/validators/conditions, reflecting the module restructuring.
6-6: Component import path aligns with new structureImporting
MetadataConditionRenderfrom"../filter/MetadataConditionRender"matches the refactored component location.
8-11: Component renamed; verify all references are updatedThe component has been renamed to
ReleaseMetadataConditionRender. Please ensure that all imports and usages of the old component nameMetadataConditionRenderhave been updated throughout the codebase to prevent import or usage errors.Run the following script to find any remaining references to
MetadataConditionRender:packages/db/src/schema/release.ts (3)
4-5: Updated imports to include release conditionsThe import statements correctly include
ReleaseConditionandVersionConditionfrom@ctrlplane/validators/releases, which are necessary for the new release filtering functionality.
Line range hint
43-45: Ensure proper handling ofreleaseDependenciesincreateReleaseschemaThe
createReleaseschema now includes thereleaseDependenciesarray. Please verify that when creating a release, the associated release dependencies are correctly validated and inserted into the database, maintaining referential integrity.
Line range hint
102-114: Verify recursion depth and stack safety inbuildConditionfunctionThe
buildConditionfunction uses recursion to handle nested conditions. For deeply nested condition trees, this could lead to stack overflow errors. Please ensure that the recursion depth is controlled or consider rewriting the function to use an iterative approach to enhance reliability.
| const value = z.string().refine((v) => !isNaN(new Date(v).getTime()), { | ||
| message: "Invalid date", | ||
| }); |
There was a problem hiding this comment.
Improve date validation robustness.
- Replace unsafe
isNaNwithNumber.isNaNto avoid type coercion issues. - Consider adding reasonable date range validation to prevent invalid dates far in the past/future.
Apply this fix for the immediate issue:
-const value = z.string().refine((v) => !isNaN(new Date(v).getTime()), {
+const value = z.string().refine((v) => {
+ const timestamp = new Date(v).getTime();
+ return !Number.isNaN(timestamp);
+}, {
message: "Invalid date",
});Consider adding additional validation:
const value = z.string().refine(
(v) => {
const date = new Date(v);
const timestamp = date.getTime();
return (
!Number.isNaN(timestamp) &&
timestamp > new Date('2000-01-01').getTime() &&
timestamp < new Date('2100-01-01').getTime()
);
},
{
message: "Invalid date or date outside allowed range (2000-2100)",
}
);🧰 Tools
🪛 Biome
[error] 10-10: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| export const createdAtCondition = z.object({ | ||
| type: createdAt, | ||
| operator, | ||
| value, | ||
| }); | ||
|
|
||
| export type CreatedAtCondition = z.infer<typeof createdAtCondition>; | ||
|
|
||
| export const updatedAtCondition = z.object({ | ||
| type: updatedAt, | ||
| operator, | ||
| value, | ||
| }); | ||
|
|
||
| export type UpdatedAtCondition = z.infer<typeof updatedAtCondition>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing duplication in condition schemas.
The createdAtCondition and updatedAtCondition schemas share identical structure, differing only in their type field.
Consider creating a generic date condition factory:
const createDateCondition = (type: typeof createdAt | typeof updatedAt) =>
z.object({
type,
operator,
value,
});
export const createdAtCondition = createDateCondition(createdAt);
export const updatedAtCondition = createDateCondition(updatedAt);
export type CreatedAtCondition = z.infer<typeof createdAtCondition>;
export type UpdatedAtCondition = z.infer<typeof updatedAtCondition>;| const options = (kinds.data ?? []).map((kind) => ({ | ||
| key: kind, | ||
| value: kind, | ||
| display: kind, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing the options mapping
The options mapping is recreated on every render, which could impact performance if there are many kinds.
Consider using useMemo to optimize performance:
- const options = (kinds.data ?? []).map((kind) => ({
- key: kind,
- value: kind,
- display: kind,
- }));
+ const options = useMemo(
+ () =>
+ (kinds.data ?? []).map((kind) => ({
+ key: kind,
+ value: kind,
+ display: kind,
+ })),
+ [kinds.data]
+ );Don't forget to add the import:
+ import { useMemo } from "react";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const options = (kinds.data ?? []).map((kind) => ({ | |
| key: kind, | |
| value: kind, | |
| display: kind, | |
| })); | |
| const options = useMemo( | |
| () => | |
| (kinds.data ?? []).map((kind) => ({ | |
| key: kind, | |
| value: kind, | |
| display: kind, | |
| })), | |
| [kinds.data] | |
| ); |
| @@ -1,11 +1,11 @@ | |||
| import { z } from "zod"; | |||
|
|
|||
| import type { MetadataCondition } from "../../conditions/index.js"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Import path inconsistency detected in releases/conditions/comparison-condition.ts
The import path in packages/validators/src/releases/conditions/comparison-condition.ts needs to be updated from ../../conditions/metadata-condition.js to ../../conditions/index.js to maintain consistency with the rest of the codebase. All other files have been properly updated to use the barrel export pattern through index.js.
packages/validators/src/releases/conditions/comparison-condition.ts: Update import path to use../../conditions/index.js
🔗 Analysis chain
Verify the import path changes across the codebase.
The import path changes align with the PR objective to extract and centralize filter-related code. Let's verify that all references are updated consistently.
Also applies to: 7-7
The results show an inconsistency in the import paths. While most files have been updated to use ../../conditions/index.js, there's one file (packages/validators/src/releases/conditions/comparison-condition.ts) that still uses the old path ../../conditions/metadata-condition.js. Let me verify if this is the only inconsistency by checking the entire import structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that metadata condition imports are consistently updated across the codebase
# and no old import paths remain.
# Test 1: Check for any remaining old import paths
echo "Checking for any remaining old imports from ./metadata-condition.js"
rg "from ['\"].*metadata-condition\.js['\"]"
# Test 2: Verify new import pattern usage
echo "Verifying new import pattern from conditions/index.js"
rg "from ['\"].*conditions/index\.js['\"]"
Length of output: 1670
Script:
#!/bin/bash
# Get a complete picture of the metadata-condition related imports and exports
echo "Checking all metadata-condition related imports and exports:"
rg -A 1 "metadata-condition" --type ts
# Check the structure of the conditions directory
echo -e "\nChecking conditions directory structure:"
fd . packages/validators/src/conditions/
Length of output: 913
| <ChoiceConditionRender | ||
| type="provider" | ||
| onSelect={setProvider} | ||
| selected={selectedProvider?.name ?? null} | ||
| options={options} | ||
| className={className} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and loading state handling.
The component implementation is clean, but could benefit from:
- Using a const enum or union type for the "provider" type string
- Passing the loading state to
ChoiceConditionRender
Consider defining a type for the condition types:
export const enum ConditionType {
Provider = 'provider',
// ... other types
}Then update the component:
<ChoiceConditionRender
- type="provider"
+ type={ConditionType.Provider}
onSelect={setProvider}
selected={selectedProvider?.name ?? null}
options={options}
className={className}
+ isLoading={providers.isLoading}
/>| @@ -0,0 +1,95 @@ | |||
| import type { DateOperator } from "@ctrlplane/validators/conditions"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Type mismatch between imported and used operators
The component has a critical type inconsistency:
- It imports
DateOperatortype but usesReleaseOperatorenum values in the SelectItem components - While both enums have the same date-related values (
Before,After,BeforeOrOn,AfterOrOn), usingReleaseOperatoris incorrect as it includes additional unrelated operators likeEquals,Like, etc. - The props expect
DateOperatortype but the component passesReleaseOperatorvalues, which could cause type errors
Fix by:
- Remove the
ReleaseOperatorimport - Use
DateOperatorenum values in the SelectItem components:
<SelectItem value={DateOperator.Before}>before</SelectItem>
<SelectItem value={DateOperator.After}>after</SelectItem>
<SelectItem value={DateOperator.BeforeOrOn}>before or on</SelectItem>
<SelectItem value={DateOperator.AfterOrOn}>after or on</SelectItem>🔗 Analysis chain
Verify operator type consistency
The component uses DateOperator type in props but imports and uses ReleaseOperator enum values. This might lead to type mismatches or confusion. Consider using consistent operator types throughout the component.
Let's verify the operator types in both modules:
Also applies to: 15-15
Let me search for more information about the DateOperator type since it wasn't found in the previous search.
Let me check the ReleaseOperator enum values to compare with the DateOperator usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the definitions of both operator types
rg -A 5 "export (type DateOperator|enum ReleaseOperator)"
Length of output: 568
Script:
#!/bin/bash
# Search for DateOperator in the entire codebase
rg -g '!*.{lock,md}' -B2 -A2 "DateOperator"
# Also check the actual usage in the component
cat "apps/webservice/src/app/[workspaceSlug]/_components/filter/DateConditionRender.tsx"
Length of output: 6022
Script:
#!/bin/bash
# Check ReleaseOperator definition
rg -A 10 "export enum ReleaseOperator" packages/validators/src/releases/conditions/release-condition.ts
# Check DateOperator definition
rg -A 10 "export enum DateOperator" packages/validators/src/conditions/date-condition.ts
Length of output: 643
| ) : ( | ||
| <div className="col-span-4 h-9 cursor-not-allowed rounded-r-md bg-neutral-900 bg-opacity-50" /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve disabled state styling and accessibility.
The disabled state could be more accessible and visually consistent.
- <div className="col-span-4 h-9 cursor-not-allowed rounded-r-md bg-neutral-900 bg-opacity-50" />
+ <div
+ className="col-span-4 h-9 cursor-not-allowed rounded-r-md bg-neutral-900 bg-opacity-50"
+ aria-disabled="true"
+ role="presentation"
+ />📝 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.
| ) : ( | |
| <div className="col-span-4 h-9 cursor-not-allowed rounded-r-md bg-neutral-900 bg-opacity-50" /> | |
| )} | |
| ) : ( | |
| <div | |
| className="col-span-4 h-9 cursor-not-allowed rounded-r-md bg-neutral-900 bg-opacity-50" | |
| aria-disabled="true" | |
| role="presentation" | |
| /> | |
| )} |
| const setOperator = ( | ||
| operator: | ||
| | MetadataOperator.Equals | ||
| | MetadataOperator.Like | ||
| | MetadataOperator.Regex | ||
| | MetadataOperator.Null, | ||
| ) => | ||
| operator === MetadataOperator.Null | ||
| ? onChange({ ...condition, operator, value: undefined }) | ||
| : onChange({ ...condition, operator, value: condition.value ?? "" }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify operator type annotation using union type alias.
The operator type annotation is verbose and could be simplified for better maintainability.
+type MetadataOperatorType =
+ | MetadataOperator.Equals
+ | MetadataOperator.Like
+ | MetadataOperator.Regex
+ | MetadataOperator.Null;
- const setOperator = (
- operator:
- | MetadataOperator.Equals
- | MetadataOperator.Like
- | MetadataOperator.Regex
- | MetadataOperator.Null,
- ) =>
+ const setOperator = (operator: MetadataOperatorType) =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setOperator = ( | |
| operator: | |
| | MetadataOperator.Equals | |
| | MetadataOperator.Like | |
| | MetadataOperator.Regex | |
| | MetadataOperator.Null, | |
| ) => | |
| operator === MetadataOperator.Null | |
| ? onChange({ ...condition, operator, value: undefined }) | |
| : onChange({ ...condition, operator, value: condition.value ?? "" }); | |
| type MetadataOperatorType = | |
| | MetadataOperator.Equals | |
| | MetadataOperator.Like | |
| | MetadataOperator.Regex | |
| | MetadataOperator.Null; | |
| const setOperator = (operator: MetadataOperatorType) => | |
| operator === MetadataOperator.Null | |
| ? onChange({ ...condition, operator, value: undefined }) | |
| : onChange({ ...condition, operator, value: condition.value ?? "" }); |
| @@ -12,7 +12,7 @@ import { | |||
| TooltipProvider, | |||
| TooltipTrigger, | |||
| } from "@ctrlplane/ui/tooltip"; | |||
| import { ReservedMetadataKey } from "@ctrlplane/validators/targets"; | |||
| import { ReservedMetadataKey } from "@ctrlplane/validators/conditions"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Multiple files still import from the old path @ctrlplane/validators/targets
The verification reveals that while ReservedMetadataKey has been moved to the new path, there are still numerous files importing various types and functions from the old path @ctrlplane/validators/targets. This suggests an incomplete migration that needs attention.
Key files still using the old path:
- Multiple components in
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ - Database schema files in
packages/db/src/schema/ - API routes and job dispatch utilities
- Integration files
🔗 Analysis chain
LGTM! Verify enum consistency across the codebase.
The import path change aligns with the PR objective of extracting reusable filter components. However, let's verify that all usages of ReservedMetadataKey have been updated consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all imports of ReservedMetadataKey have been updated
# and that no references to the old path remain.
echo "Checking for any remaining imports from the old path..."
rg "from ['\"]@ctrlplane/validators/targets['\"]"
echo "Verifying all current imports use the new path..."
rg "from ['\"]@ctrlplane/validators/conditions['\"]"
Length of output: 9330
| <MetadataConditionRender | ||
| condition={condition} | ||
| onChange={onChange} | ||
| metadataKeys={metadataKeys} | ||
| className={className} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle loading and error states for metadata keys
The metadataKeys may be undefined or empty if the data is still loading or if there was an error in the API calls. Consider adding loading and error handling to improve user experience.
You could add a loading state and error handling like this:
return (
+ {workspaceMetadataKeys.isLoading || systemMetadataKeys.isLoading ? (
+ <div>Loading metadata keys...</div>
+ ) : (workspaceMetadataKeys.isError || systemMetadataKeys.isError ? (
+ <div>Error loading metadata keys</div>
+ ) : (
<MetadataConditionRender
condition={condition}
onChange={onChange}
metadataKeys={metadataKeys}
className={className}
/>
+ ))}
);Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
ChoiceConditionRender,DateConditionRender,MetadataConditionRender,ReleaseMetadataConditionRender, andTargetMetadataConditionRenderfor enhanced filtering and selection functionalities.ReservedMetadataKeyfrom@ctrlplane/validators/conditions, improving metadata handling.KindConditionRenderandProviderConditionRenderby integratingChoiceConditionRender.