fix: Init job filters #174
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 54
🧹 Outside diff range and nitpick comments (35)
packages/validators/src/jobs/conditions/deployment-condition.ts (2)
3-7: Add JSDoc documentation to explain the schema's purpose.Consider adding documentation to explain the purpose and usage of this schema. This will help other developers understand how to use it correctly.
+/** + * Validates deployment conditions for jobs. + * Used to ensure that job triggers have valid deployment conditions + * with proper type, operator, and deployment UUID. + */ export const deploymentCondition = z.object({ type: z.literal("deployment"), operator: z.literal("equals"), value: z.string().uuid(), -}); +}).describe('Deployment condition for job triggers');
5-5: Consider supporting additional comparison operators.The operator field is currently restricted to "equals". Consider if other operators like "not_equals" might be useful for more flexible deployment condition matching.
packages/validators/src/jobs/conditions/environment-condition.ts (1)
3-7: Consider adding JSDoc documentation for the schema.Adding documentation would help explain the purpose and usage of this environment condition schema to other developers.
+/** + * Validates environment conditions for jobs. + * Used to specify environment-specific job triggers or constraints. + * @example + * { + * type: "environment", + * operator: "equals", + * value: "123e4567-e89b-12d3-a456-426614174000" + * } + */ export const environmentCondition = z.object({apps/webservice/src/app/[workspaceSlug]/_components/job-condition/job-condition-props.ts (1)
1-9: LGTM! Consider adding JSDoc comments.The type definition is well-structured and follows TypeScript best practices. The generic type parameter ensures type safety while allowing flexibility for different condition types.
Consider adding JSDoc comments to document the purpose of the type and its properties:
+/** + * Props for components that render job conditions. + * @template T - The specific type of JobCondition being rendered + */ export type JobConditionRenderProps<T extends JobCondition> = { + /** The job condition to render */ condition: T; + /** Callback fired when the condition changes */ onChange: (condition: T) => void; + /** Optional callback to handle condition removal */ onRemove?: () => void; + /** Optional depth level for nested conditions */ depth?: number; + /** Optional CSS class name for styling */ className?: string; };packages/validators/src/conditions/index.ts (1)
17-22: Consider consistent naming convention for enum values.While the enum is well-structured, consider using consistent naming for values. Currently, 'created-at' uses kebab-case while others are single words.
export enum FilterType { Metadata = "metadata", - CreatedAt = "created-at", + CreatedAt = "createdat", Comparison = "comparison", Version = "version", }apps/webservice/src/app/[workspaceSlug]/_components/release-condition/VersionConditionRender.tsx (1)
10-24: Consider adding documentation to clarify component relationships.While the implementation is clean, it would be helpful to add JSDoc comments explaining:
- The purpose of this wrapper component
- The relationship between
ReleaseVersionConditionRenderand the childVersionConditionRender- Why this adaptation layer is necessary
Add documentation like this:
+/** + * Adapts the generic VersionConditionRender component for use within the release condition context. + * This wrapper ensures type safety and proper state management for version conditions specifically + * within releases. + */ export const ReleaseVersionConditionRender: React.FC<apps/webservice/src/app/[workspaceSlug]/_components/job-condition/VersionConditionRender.tsx (1)
23-23: Consider making the title configurable.The title "Release version" is hardcoded. Consider making it configurable through props to support reusability in different contexts.
- title="Release version" + title={title ?? "Release version"}And update the props interface:
interface JobReleaseVersionConditionRenderProps extends JobConditionRenderProps<VersionCondition> { title?: string; }apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobsTable.tsx (2)
17-20: LGTM! Consider handling error and loading states.The parameter change to useQuery is correct and aligns with the standardization across components.
Consider enhancing the component with error and loading states:
const releaseJobTriggers = api.job.config.byReleaseId.useQuery( { releaseId }, { refetchInterval: 10_000 }, ); + + if (releaseJobTriggers.isLoading) { + return <div>Loading jobs...</div>; + } + + if (releaseJobTriggers.isError) { + return <div>Error loading jobs: {releaseJobTriggers.error.message}</div>; + } + + if (!releaseJobTriggers.data?.length) { + return <div>No jobs found</div>; + }
Line range hint
24-24: Add column headers to improve table readability.The TableHeader is currently empty but contains relevant columns (Environment, Target, Status, Type).
Consider adding headers:
-<TableHeader></TableHeader> +<TableHeader> + <TableRow> + <TableCell>Environment</TableCell> + <TableCell>Target</TableCell> + <TableCell>Status</TableCell> + <TableCell>Type</TableCell> + </TableRow> +</TableHeader>packages/validators/src/conditions/date-condition.ts (1)
23-27: Consider using TypeScript's built-in enum value type.While the explicit
DateOperatorTypeis valid and provides clear documentation of allowed values, you could alternatively use TypeScript's built-inDateOperator[keyof typeof DateOperator]to automatically derive the union type from the enum, ensuring the type stays in sync with enum changes.Here's an alternative implementation:
-export type DateOperatorType = - | DateOperator.Before - | DateOperator.After - | DateOperator.BeforeOrOn - | DateOperator.AfterOrOn; +export type DateOperatorType = DateOperator[keyof typeof DateOperator];Both approaches are valid - the current explicit approach might be preferred for readability and documentation purposes, while the suggested approach would be more maintainable as it automatically stays in sync with the enum.
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobCreatedAtConditionRender.tsx (1)
24-33: Consider extracting the type string as a constant.The "Created at" string is hardcoded in the render method. For better maintainability and reusability, consider extracting it to a constant or enum.
+const JOB_CONDITION_TYPE = { + CREATED_AT: "Created at" +} as const; + export const JobCreatedAtConditionRender: React.FC< JobConditionRenderProps<CreatedAtCondition> > = ({ condition, onChange, className }) => { // ... existing code ... return ( <DateConditionRender setDate={setDate} setOperator={setOperator} value={condition.value} operator={condition.operator} - type="Created at" + type={JOB_CONDITION_TYPE.CREATED_AT} className={className} /> ); };apps/webservice/src/app/[workspaceSlug]/_components/job-condition/StatusConditionRender.tsx (1)
12-14: Consider adding JSDoc documentation for the component.Adding JSDoc documentation would improve maintainability by clearly describing the component's purpose and its props.
+/** + * Renders a selection interface for job statuses. + * @param {JobConditionRenderProps<StatusCondition>} props - The component props + * @param {StatusCondition} props.condition - The current status condition + * @param {Function} props.onChange - Callback function when status changes + * @param {string} [props.className] - Optional CSS class name + */ export const StatusConditionRender: React.FC< JobConditionRenderProps<StatusCondition> > = ({ condition, onChange, className }) => {apps/webservice/src/app/[workspaceSlug]/_components/target-condition/KindConditionRender.tsx (1)
Line range hint
19-24: Consider adding error state handling.While the loading states are well handled, it would be beneficial to handle error states from both queries to provide better feedback to users when data fetching fails.
Consider adding error handling like this:
const options = (kinds.data ?? []).map((kind) => ({ key: kind, value: kind, display: kind, })); + const error = workspace.error || kinds.error; const loading = workspace.isLoading || kinds.isLoading; return ( <ChoiceConditionRender type="kind" onSelect={setKind} selected={condition.value} options={options} className={className} loading={loading} + error={error?.message} /> );apps/webservice/src/app/[workspaceSlug]/_components/JobTableStatusIcon.tsx (1)
30-33: LGTM! Consider grouping related status checks.The implementation for cancelled status is well-done, using appropriate visual indicators and following the established patterns. The neutral color choice effectively distinguishes it from failure states while using the same icon.
Consider grouping related status checks together for better code organization. For example, you could move this check next to the failure status check since they share the same icon:
if (status === JobStatus.Failure || status === JobStatus.InvalidJobAgent) return <IconCircleX className={cn("h-4 w-4 text-red-400", className)} />; + if (status === JobStatus.Cancelled) + return <IconCircleX className={cn("h-4 w-4 text-neutral-400", className)} />; if (status === JobStatus.Pending) return <IconClock className={cn("h-4 w-4 text-neutral-400", className)} />; if (status === JobStatus.InProgress) return ( <div className="animate-spin rounded-full text-blue-400"> <IconLoader2 className={cn("h-4 w-4", className)} /> </div> ); - if (status === JobStatus.Cancelled) - return ( - <IconCircleX className={cn("h-4 w-4 text-neutral-400", className)} /> - );apps/webservice/src/app/[workspaceSlug]/_components/job-condition/useJobFilter.ts (1)
10-18: Consider enhancing error handling with specific error types.While the current error handling works, it could be more informative for debugging purposes.
Consider this enhancement:
try { return JSON.parse(LZString.decompressFromEncodedURIComponent(filterJson)); } catch (error) { + console.warn('Failed to parse job filter:', error); return undefined; }apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ProviderConditionRender.tsx (1)
31-32: Consider using useMemo for loading state.The loading state implementation is correct, but since it depends on reactive values, consider memoizing it to prevent unnecessary recalculations.
- const loading = workspace.isLoading || providers.isLoading; + const loading = useMemo( + () => workspace.isLoading || providers.isLoading, + [workspace.isLoading, providers.isLoading] + );packages/validators/src/jobs/conditions/comparison-condition.ts (1)
1-52: Consider adding depth limit for nested conditions.The recursive nature of comparison conditions could lead to performance issues or stack overflow with deeply nested structures. Consider adding a maximum depth limit to prevent potential DoS vectors.
You could add depth validation in the Zod schema by:
- Adding a
maxDepthparameter to track nesting level- Throwing an error when it exceeds a threshold (e.g., 10 levels)
Would you like me to provide an implementation example?
apps/webservice/src/app/[workspaceSlug]/_components/filter/VersionConditionRender.tsx (2)
14-21: Consider adding JSDoc comments and value validation.While the type definition is well-structured, consider these improvements:
- Add JSDoc comments to document the purpose and usage of each prop
- Consider adding validation constraints for the
valueprop based on the selected operatorExample enhancement:
+/** + * Props for the VersionConditionRender component + * @property operator - The comparison operator for version matching + * @property value - The version value to compare against + * @property setOperator - Callback to update the operator + * @property setValue - Callback to update the value + * @property className - Optional CSS class names + * @property title - Optional custom title, defaults to "Version" + */ type VersionConditionRenderProps = { operator: VersionOperatorType; value: string; setOperator: (operator: VersionOperatorType) => void; setValue: (value: string) => void; className?: string; title?: string; };
31-35: Enhance accessibility with ARIA labels.The title div should be properly associated with its related input fields for better accessibility.
-<div className="col-span-2 flex items-center rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground"> +<div + className="col-span-2 flex items-center rounded-l-md border bg-transparent px-3 text-sm text-muted-foreground" + id="version-condition-label" + aria-label={`${title} condition filter`} +> {title} </div>apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionDialog.tsx (1)
42-45: Consider using responsive width classes.The hardcoded min-width might not be ideal for all screen sizes. Consider using Tailwind's responsive width classes instead.
- <DialogContent - className="min-w-[1000px]" - onClick={(e) => e.stopPropagation()} - > + <DialogContent + className="w-full max-w-[90vw] md:max-w-[1000px]" + onClick={(e) => e.stopPropagation()} + >apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionRender.tsx (2)
1-22: Consider organizing imports into clear groupsThe imports could be organized into clearer groups for better maintainability:
- External dependencies (React, validators)
- Types/interfaces
- Internal components
import type { JobCondition } from "@ctrlplane/validators/jobs"; import React from "react"; - import { isComparisonCondition, isCreatedAtCondition, isDeploymentCondition, isEnvironmentCondition, isMetadataCondition, isStatusCondition, isVersionCondition, } from "@ctrlplane/validators/jobs"; + import type { JobConditionRenderProps } from "./job-condition-props"; + import { DeploymentConditionRender } from "./DeploymentConditionRender"; import { EnvironmentConditionRender } from "./EnvironmentConditionRender";
23-29: Enhance component documentationWhile the minimum width requirement is noted, the documentation could be more comprehensive:
- Explain why 1000px is required
- Document each prop's purpose
- Add example usage
/** - * The parent container should have min width of 1000px - * to render this component properly. + * A component that renders different types of job conditions based on their type. + * + * @requires The parent container should have min width of 1000px to prevent layout issues + * with nested condition renderers. + * + * @param condition - The job condition to render + * @param onChange - Callback when condition is modified + * @param onRemove - Callback when condition is removed + * @param depth - Nesting level for indentation (default: 0) + * @param className - Additional CSS classes + * + * @example + * <JobConditionRender + * condition={jobCondition} + * onChange={handleChange} + * onRemove={handleRemove} + * /> */apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (2)
61-69: Consider maintaining consistent text casing.The loading message could be made consistent with the search placeholder format.
- Loading {type}s... + Loading {capitalCase(type)}s...
Line range hint
56-85: Consider extracting command list logic for reusability.The command list implementation could be extracted into a separate component to promote reusability across similar filter components.
This would:
- Reduce code duplication if other filters need similar loading/empty states
- Make it easier to maintain consistent UX across filters
- Simplify testing of the command list behavior
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx (1)
Line range hint
165-175: Consider extracting common condition badge pattern.While the implementation is correct, there's an opportunity to reduce code duplication across condition components by extracting the common pattern.
Consider creating a reusable component:
interface BaseConditionProps { label: string; operator: keyof typeof operatorVerbs; value?: React.ReactNode; } const BaseConditionBadge: React.FC<BaseConditionProps> = ({ label, operator, value }) => ( <ConditionBadge> <span className="text-white">{label}</span> <span className="text-muted-foreground"> {operatorVerbs[operator]} </span> {value != null && <span className="text-white">{value}</span>} </ConditionBadge> ); // Then simplify the version condition to: const StringifiedVersionCondition: React.FC<{ condition: VersionCondition; }> = ({ condition }) => ( <BaseConditionBadge label="version" operator={condition.operator} value={condition.value} /> );packages/db/src/schema/release.ts (1)
232-234: Consider adding explicit operator check for regex case.While the logic is correct, consider making the regex case more explicit for better maintainability.
const buildVersionCondition = (cond: VersionCondition): SQL => { if (cond.operator === VersionOperator.Equals) return eq(release.version, cond.value); if (cond.operator === VersionOperator.Like) return like(release.version, cond.value); - return sql`${release.version} ~ ${cond.value}`; + if (cond.operator === VersionOperator.Regex) + return sql`${release.version} ~ ${cond.value}`; + throw new Error(`Unsupported version operator: ${cond.operator}`); };apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (1)
37-37: Enhance accessibility and user experience of the filter button.Consider adding accessibility attributes and visual indicators:
- <Button variant="ghost" size="icon" className="flex h-7 w-7 gap-1"> + <Button + variant="ghost" + size="icon" + className="flex h-7 w-7 gap-1 hover:bg-neutral-800/50" + aria-label="Filter jobs" + title="Filter jobs"> <IconFilter className="h-4 w-4" /> </Button>Also applies to: 41-42, 53-60
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1)
167-170: Consider documenting the refetch interval rationale.The query parameter restructuring and addition of the refetch interval look good. However, it would be helpful to document why 10 seconds was chosen as the interval, especially since this affects the UI update frequency and API load.
Add a comment explaining the rationale:
const allJobs = api.job.config.byReleaseId.useQuery( { releaseId: release.id }, + // Refresh job statuses every 10s to balance UI responsiveness and API load { refetchInterval: 10_000 }, );packages/api/src/router/deployment.ts (1)
140-154: Consider explicit "not found" handling.For better error handling and consistency with the
bySlugmethod, consider usingtakeFirstOrNulland explicitly returningnullwhen the deployment is not found.Example implementation:
byId: protectedProcedure .input(z.string().uuid()) .meta({ authorizationCheck: ({ canUser, input }) => canUser .perform(Permission.DeploymentGet) .on({ type: "deployment", id: input }), }) .query(({ ctx, input }) => ctx.db .select() .from(deployment) .where(eq(deployment.id, input)) - .then(takeFirst), + .then(takeFirstOrNull), }),apps/webservice/src/app/[workspaceSlug]/_components/job-condition/EnvironmentConditionRender.tsx (1)
39-43: Handle potential errors in loading stateWhile aggregating the loading states using logical OR (
||) works, consider also handling potential errors from the queries to provide a better user experience.You might extend the loading state to include error checks:
const loading = workspaceQ.isLoading || systemQ.isLoading || workspaceEnvironmentsQ.isLoading || systemEnvironmentsQ.isLoading; +const error = + workspaceQ.error || + systemQ.error || + workspaceEnvironmentsQ.error || + systemEnvironmentsQ.error; +if (error) { + // Handle the error accordingly +}packages/validators/src/jobs/conditions/job-condition.ts (1)
45-50: EnsuredefaultConditionaligns with expected defaultsThe
defaultConditioninitializes a comparison condition with an emptyconditionsarray. Depending on how it's used, this might not be meaningful or could lead to unintended behavior if consumers of this default expect certain conditions.Review the usage of
defaultConditionto ensure it meets the expectations of the consuming code. If necessary, consider adding a default condition within theconditionsarray or provide guidance on how it should be used.apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx (1)
133-133: Consider Debouncing theclearFunctionIf the
clearoperation is triggered frequently, consider debouncing it to prevent unnecessary re-renders and improve performance.You might implement debouncing as follows:
- const clear = () => onChange({ ...condition, conditions: [] }); + const clear = debounce(() => onChange({ ...condition, conditions: [] }), 300);Remember to import a debounce utility, such as from
lodash.packages/api/src/router/job.ts (3)
24-24: Maintain Consistent Import Ordering for ReadabilityThe import of
countDistinctat line 24 is positioned betweenascanddesc. For improved readability and to adhere to coding conventions, consider ordering the imports alphabetically or grouping related imports together.
157-158: Adjust Indentation for Authorization CheckLines 157-158 have indentation that may not align with the project's coding style. Proper indentation improves code readability.
Consider aligning the
.performand.onmethods with the indentation level of the surrounding code.
687-687: Rename Export for ConsistencyAt line 687,
metadataKeyis used as the key when exportingmetadataKeysRouter. For consistency and clarity, consider renaming it tometadataKeys.Apply this change:
export const jobRouter = createTRPCRouter({ // other routers - metadataKey: metadataKeysRouter, + metadataKeys: metadataKeysRouter, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (48)
- apps/webservice/src/app/[workspaceSlug]/(job)/jobs/JobTable.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/(job)/jobs/page.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/JobTableStatusIcon.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (6 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/VersionConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/DeploymentConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/EnvironmentConditionRender.tsx (1 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/JobConditionDialog.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobCreatedAtConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobMetadataConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/StatusConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/VersionConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/job-condition-props.ts (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/useJobFilter.ts (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ComparisonConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx (3 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseCreatedAtConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/VersionConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/KindConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ProviderConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowNode.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/JobsTable.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (4 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/page.tsx (0 hunks)
- packages/api/src/router/deployment.ts (1 hunks)
- packages/api/src/router/environment.ts (1 hunks)
- packages/api/src/router/job.ts (10 hunks)
- packages/db/src/schema/job.ts (3 hunks)
- packages/db/src/schema/release.ts (3 hunks)
- packages/validators/src/auth/index.ts (1 hunks)
- packages/validators/src/conditions/date-condition.ts (1 hunks)
- packages/validators/src/conditions/index.ts (1 hunks)
- packages/validators/src/conditions/version-condition.ts (1 hunks)
- packages/validators/src/jobs/conditions/comparison-condition.ts (1 hunks)
- packages/validators/src/jobs/conditions/deployment-condition.ts (1 hunks)
- packages/validators/src/jobs/conditions/environment-condition.ts (1 hunks)
- packages/validators/src/jobs/conditions/index.ts (1 hunks)
- packages/validators/src/jobs/conditions/job-condition.ts (1 hunks)
- packages/validators/src/jobs/conditions/status-condition.ts (1 hunks)
- packages/validators/src/jobs/index.ts (2 hunks)
- packages/validators/src/releases/conditions/comparison-condition.ts (1 hunks)
- packages/validators/src/releases/conditions/index.ts (0 hunks)
- packages/validators/src/releases/conditions/release-condition.ts (1 hunks)
💤 Files with no reviewable changes (2)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/page.tsx
- packages/validators/src/releases/conditions/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/validators/src/jobs/conditions/index.ts
🔇 Additional comments (73)
packages/validators/src/jobs/conditions/deployment-condition.ts (1)
9-9: LGTM!The type export using
z.inferis the correct approach and follows TypeScript naming conventions.packages/validators/src/jobs/conditions/environment-condition.ts (3)
9-9: LGTM!The type inference from the Zod schema ensures type safety and consistency between runtime validation and compile-time types.
3-7: Consider expanding operator options for more flexibility.The schema currently only supports the "equals" operator. Consider if other comparison operators might be useful for environment conditions (e.g., "not_equals", "in", "not_in").
Let's check if other condition types support multiple operators:
1-9: Consider runtime validation of environment existence.While the UUID format is validated, there's no runtime validation to ensure the environment actually exists in the system. Consider:
- Adding a custom validator to check environment existence
- Creating a separate validation function for runtime checks
- Documenting the assumption that environment existence is validated elsewhere
This would help prevent references to non-existent environments.
Let's check if there's any existing environment validation:
packages/validators/src/conditions/version-condition.ts (1)
11-15: Consider if VersionOperator enum adds value over string literals.While the enum provides a centralized place for constants, it duplicates values already defined in the zod schema. Consider if this enum is necessary, as the same type safety can be achieved through the existing zod schema and TypeScript literal types.
Let's check if this enum is actually used for enum-specific features (like iteration):
packages/validators/src/jobs/conditions/status-condition.ts (1)
22-22: LGTM!The type export correctly uses Zod's inference to maintain type safety and schema consistency.
packages/validators/src/conditions/index.ts (4)
3-3: LGTM! Export statement follows established patterns.The new version condition export follows the same pattern as other condition exports in the file.
12-15: LGTM! Logical operators are well-defined.The ComparisonOperator enum provides clear and standard logical operators.
24-24: Verify the impact and usage of depth limitation.While the constant is well-defined, let's understand how this depth limitation is enforced and its impact on nested conditions/filters.
#!/bin/bash # Search for usage of MAX_DEPTH_ALLOWED rg "MAX_DEPTH_ALLOWED" --type ts -A 5 -B 5
5-10: Verify input validation for 'like' and 'regex' operators.While the enum definition is clean and well-structured, the 'like' and 'regex' operators could potentially lead to performance issues if not properly validated.
Let's check if validation exists:
✅ Verification successful
Input validation exists for 'like' and 'regex' operators
The codebase shows proper validation using Zod schemas for both operators:
- Metadata conditions validate through
likeConditionandregexConditionschemas with required string values- Version conditions validate through
versionConditionschema with string length checks- Name conditions use
nameConditionschema with string validationThe validation ensures:
- Non-empty strings (
.min(1))- Type safety through literal operator types
- Proper schema composition with
z.unionfor combining conditions🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for validation logic related to these operators rg -l "like|regex" --type ts packages/validators/Length of output: 15112
Script:
#!/bin/bash # Search for validation logic related to like and regex operators rg -A 5 -B 5 "like|regex" --type ts packages/validators/Length of output: 8127
Script:
#!/bin/bash # Look for any validation functions or classes in the validators package fd -e ts . packages/validators/ --exec ast-grep --pattern 'function $FUNC($$$) { $$$ }'Length of output: 91
Script:
#!/bin/bash # Search for any validation related to column operators rg -A 5 -B 5 "ColumnOperator" --type ts packages/validators/Length of output: 724
apps/webservice/src/app/[workspaceSlug]/(job)/jobs/page.tsx (3)
5-5: LGTM!The import statement for the new JobTable component is correctly structured.
15-17: LGTM! Good improvement to the API call structure.Using an object parameter makes the API more maintainable and extensible for future additions.
19-19: Verify loading state handling in JobTable component.The simplified rendering looks good, but let's ensure the JobTable component properly handles loading states to prevent potential UI flickers or empty states.
Also applies to: 21-21
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/VersionConditionRender.tsx (1)
1-24: LGTM! Clean and well-structured implementation.The changes demonstrate good practices:
- Proper TypeScript usage with clear type imports
- Clean state management with immutable updates
- Efficient prop forwarding to child component
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/VersionConditionRender.tsx (1)
1-26: LGTM! Clean and well-typed implementation.The component is well-structured with proper type safety and immutable state updates.
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobMetadataConditionRender.tsx (2)
1-12: LGTM! Clean component setup and type definitions.The component is well-structured with proper type definitions and imports. The use of generics with JobConditionRenderProps is a good practice for type safety.
19-26:⚠️ Potential issueHandle loading and error states in the render method.
The component should provide feedback during loading and error states.
Consider enhancing the render implementation:
return ( + <> + {isLoading && <div>Loading metadata keys...</div>} + {isError && <div>Error loading metadata keys: {error.message}</div>} + {!isLoading && !isError && ( <MetadataConditionRender condition={condition} onChange={onChange} metadataKeys={metadataKeys} className={className} /> + )} + </> );Likely invalid or redundant comment.
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseCreatedAtConditionRender.tsx (2)
3-3: LGTM! Type import update enhances consistency.The change from
DateOperatortoDateOperatorTypealigns well with the broader refactoring effort to standardize type handling across condition components.
21-22: LGTM! Clean function signature update.The simplified signature using
DateOperatorTypemaintains the component's functionality while improving type safety. The implementation correctly preserves immutable state updates.apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobCreatedAtConditionRender.tsx (3)
1-9: LGTM! Imports are well-organized and properly typed.The imports are cleanly separated between external dependencies and internal modules, with all necessary types properly imported.
10-12: LGTM! Component is well-typed with proper generic constraints.The component definition follows React best practices with proper TypeScript typing.
13-22: 🛠️ Refactor suggestionConsider making timezone handling more explicit and robust.
The current implementation has potential timezone-related edge cases:
- The timezone is implicitly derived from the system's locale
- The conversion process might not be immediately clear to other developers
Let's check if there are other date handling implementations in the codebase:
Consider these improvements:
- const setDate = (t: DateValue) => - onChange({ - ...condition, - value: t - .toDate(Intl.DateTimeFormat().resolvedOptions().timeZone) - .toISOString(), - }); + const setDate = (t: DateValue) => { + // Explicitly get the user's timezone once + const userTimeZone = Intl.DateTimeFormat().resolvedOptions().timeZone; + + // Convert to ISO string while preserving the user's local timezone + const dateInUserTZ = t.toDate(userTimeZone); + + onChange({ + ...condition, + value: dateInUserTZ.toISOString(), + }); + };packages/validators/src/releases/conditions/comparison-condition.ts (2)
5-5: LGTM! Import paths are properly updated.The changes maintain a clean separation of concerns by moving condition-related files to a dedicated conditions directory. The imports are well-organized with types separated from regular imports.
Also applies to: 8-8
5-5: Verify the updated import paths.The import path changes look correct, but let's verify the file structure to ensure consistency.
Also applies to: 8-8
✅ Verification successful
Import paths are correctly updated and consistent across the codebase
The verification confirms:
- The
version-condition.tsfile exists atpackages/validators/src/conditions/version-condition.ts- All imports of
version-condition.jsacross the codebase consistently use the../../conditions/version-condition.jspath pattern- The changes align with the codebase's import structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and location of version-condition.js # and check for consistent import patterns across the codebase # Check if the file exists at the new location fd --type f "version-condition.ts" --exec echo "Found version-condition at: {}" # Check import patterns across the codebase to verify consistency rg --type ts "from ['\"].*version-condition.js['\"]"Length of output: 1428
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/StatusConditionRender.tsx (2)
1-11: LGTM! Clean and well-organized imports.The imports are properly structured with clear separation between types and values, and use specific imports rather than wildcards.
28-36: LGTM! Clean component rendering.The component properly renders the ChoiceConditionRender with all necessary props and handles the null case appropriately.
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/KindConditionRender.tsx (1)
25-26: LGTM! Good addition of loading state handling.The combined loading state implementation is clean and correctly accounts for both data fetching operations. Passing it to the ChoiceConditionRender component improves the user experience by showing loading indicators during data fetching.
Also applies to: 34-34
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/useJobFilter.ts (3)
1-5: LGTM! Well-organized imports with appropriate dependencies.The imports are clean and necessary, with good separation between external dependencies and internal types.
6-9: LGTM! Clean hook implementation following React best practices.The hook is well-structured with clear input/output interface.
Also applies to: 39-40
20-37: 🛠️ Refactor suggestionVerify SSR compatibility and enhance URL handling.
Two potential improvements to consider:
- Direct usage of
window.locationmight cause issues during server-side rendering.- The URL parameter handling could be more robust.
Consider these improvements:
const setFilter = useCallback( (filter: JobCondition | undefined) => { if (filter == null) { - const query = new URLSearchParams(window.location.search); + const query = new URLSearchParams(urlParams.toString()); query.delete("job-filter"); router.replace(`?${query.toString()}`); return; } const filterJson = LZString.compressToEncodedURIComponent( JSON.stringify(filter), ); - const query = new URLSearchParams(window.location.search); + const query = new URLSearchParams(urlParams.toString()); query.set("job-filter", filterJson); router.replace(`?${query.toString()}`); }, - [router], + [router, urlParams], );Let's verify the SSR compatibility:
apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ProviderConditionRender.tsx (1)
40-40: Verify loading prop implementation across condition components.The loading prop implementation looks good. Let's verify that this pattern is consistently implemented across other condition components.
✅ Verification successful
Based on the examination of the code, I can now provide the final response:
Loading prop is consistently implemented across condition components
The loading prop implementation follows a consistent pattern:
ChoiceConditionRenderproperly handles the loading prop with a default value and loading state UI- Components like
ProviderConditionRenderandKindConditionRendercorrectly derive and pass the loading state from their data fetching operations- Loading states are properly composed from multiple async operations (e.g.
workspace.isLoading || providers.isLoading)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other condition render components implement loading prop consistently # Expected: Find similar loading prop implementations in other condition components # Search for other condition render components and their loading prop usage ast-grep --pattern 'const $name = ({ $$$, loading }) => { $$$ }' # Search for loading prop being passed to ChoiceConditionRender rg -A 2 'loading={.*}' --type typescriptLength of output: 215
Script:
#!/bin/bash # Let's try a different approach to find loading prop implementations # Search for files containing "ConditionRender" in their name fd "ConditionRender" # Search for loading prop patterns with proper escaping rg "loading=\{" --type typescript # Search for component definitions with loading prop ast-grep --pattern 'const $_ = ({ loading }: { loading: $_ }) => {' # Alternative pattern to find loading prop in component props ast-grep --pattern 'interface $_ { loading: $_ }'Length of output: 2402
Script:
#!/bin/bash # Let's examine the content of a few condition render components to check loading prop implementation # Check ProviderConditionRender.tsx content cat "apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ProviderConditionRender.tsx" # Check a few other condition renders for comparison cat "apps/webservice/src/app/[workspaceSlug]/_components/target-condition/KindConditionRender.tsx" cat "apps/webservice/src/app/[workspaceSlug]/_components/target-condition/NameConditionRender.tsx" # Try ripgrep with correct file type rg "loading=\{" -t tsxLength of output: 4055
Script:
#!/bin/bash # Let's check the ChoiceConditionRender implementation and other components that might use loading prop # Check ChoiceConditionRender implementation cat "apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx" # Search for loading prop patterns in all files rg "loading={" --type-add 'tsx:*.tsx' -t tsx # Search for loading prop type definitions ast-grep --pattern 'interface $_ { $$$ loading: boolean $$$ }'Length of output: 3528
packages/validators/src/jobs/index.ts (1)
1-1: Verify the conditions module exists.The new export statement suggests a dependency on a conditions module. Let's ensure this module exists and is properly structured.
✅ Verification successful
Export statement is valid and properly structured
The conditions module exists at
packages/validators/src/jobs/conditions/index.tsand contains a well-organized set of condition-related files that are being exported:
- comparison-condition.ts
- deployment-condition.ts
- environment-condition.ts
- job-condition.ts
- status-condition.ts
The export statement in the review is correct and follows proper module structuring practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and structure of the conditions module # Check if the conditions module exists fd --type f "index.js" --parent-dir "conditions" --base-directory "packages/validators/src/jobs" # If it exists, check its exports if [ $? -eq 0 ]; then rg -l "export" "packages/validators/src/jobs/conditions" fiLength of output: 379
Script:
#!/bin/bash # Description: Verify the existence and structure of the conditions module using correct fd syntax # Check if the conditions directory and index file exist fd "index\.(js|ts)" "packages/validators/src/jobs/conditions" # If found, check its contents for exports rg "export" "packages/validators/src/jobs/conditions" -lLength of output: 524
packages/validators/src/jobs/conditions/comparison-condition.ts (2)
1-18: LGTM! Well-organized imports following best practices.The imports are properly organized, separating types from values, and using explicit paths with .js extensions for ESM compatibility.
39-52: LGTM! Type definition aligns perfectly with the schema.The TypeScript type accurately reflects the Zod schema structure, ensuring consistent typing across runtime validation and compile-time checks.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowNode.tsx (1)
19-22: LGTM! Query parameter structure aligns with standardization.The change to use an object parameter structure for the query is consistent with the codebase-wide standardization effort.
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionRender.tsx (1)
Line range hint
59-65: LGTM! Component usage correctly updated.The component usage has been properly updated to reflect the new naming convention, maintaining consistency with the release-specific component naming pattern.
apps/webservice/src/app/[workspaceSlug]/_components/filter/VersionConditionRender.tsx (2)
1-12: LGTM! Imports are well-organized and properly utilized.The imports are logically grouped and all are actively used in the component.
37-46: LGTM! Select implementation is well-structured.The Select component implementation is clean and provides good user experience with clear options.
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/DeploymentConditionRender.tsx (1)
1-8: LGTM! Clean and well-organized imports.The imports are properly organized with type imports separated, and all imports are being utilized.
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionDialog.tsx (2)
1-26: LGTM! Well-structured imports and type definitions.The imports are properly organized and the props interface is well-defined with clear TypeScript types.
28-87: Add test coverage for this component.As this is a crucial component for job condition management, consider adding tests to verify:
- Condition validation
- State management
- UI interaction flows
- Error handling
Let's check if there are any existing tests:
Would you like me to help create a test suite for this component?
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionRender.tsx (1)
15-21: Verify all condition render components exist and have consistent propsLet's verify that all imported condition render components exist and maintain consistent prop interfaces.
✅ Verification successful
Let me try another verification approach since the previous script had issues with TypeScript file recognition.
Based on the verification results, I can now provide the final response:
All condition render components exist and maintain consistent props
All imported condition render components are present in the codebase and follow a consistent props interface pattern with
conditionandonChangeprops. The components are:
- DeploymentConditionRender
- EnvironmentConditionRender
- JobComparisonConditionRender
- JobCreatedAtConditionRender
- JobMetadataConditionRender
- StatusConditionRender
- JobReleaseVersionConditionRender (imported from VersionConditionRender.tsx)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if all condition render components exist and verify their prop types # Look for all condition render components echo "Searching for condition render components..." for component in "DeploymentConditionRender" "EnvironmentConditionRender" "JobComparisonConditionRender" "JobCreatedAtConditionRender" "JobMetadataConditionRender" "StatusConditionRender" "JobReleaseVersionConditionRender"; do echo "Checking $component..." rg -l "$component" --type typescript done # Check prop types consistency echo "Checking prop types..." ast-grep --pattern 'interface $name Props { $$$ condition: $type; onChange: $_; $$$ }'Length of output: 3060
Script:
#!/bin/bash # Search for condition render components using regular ripgrep echo "Searching for condition render components..." for component in "DeploymentConditionRender" "EnvironmentConditionRender" "JobComparisonConditionRender" "JobCreatedAtConditionRender" "JobMetadataConditionRender" "StatusConditionRender" "JobReleaseVersionConditionRender"; do echo "=== Checking $component ===" rg "$component" -A 5 apps/webservice/src/app/ done # Look for the actual component files echo "\nSearching for component files..." fd ".*ConditionRender\.(tsx|ts)$" apps/webservice/src/app/Length of output: 21835
apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (4)
21-21: LGTM: Type definition is well-structured.The optional loading prop is properly typed and follows TypeScript conventions.
30-30: LGTM: Props handling is correct.The default value for loading is appropriately set to false, ensuring backward compatibility.
39-39: LGTM: Text change improves readability.The addition of "is" makes the filter condition read more naturally.
76-76: Verify the switch from key to value in option selection.The change from
option.keytooption.valuein theonSelecthandler might require updates in parent components.Also applies to: 78-78
✅ Verification successful
Let me analyze the component usage further to verify the impact of the change.
Change from
option.keytooption.valueis consistent with the codebaseLooking at how options are constructed across all parent components, we can see that they consistently set both
keyandvalueto the same identifier (e.g.,provider.id,deployment.id,kind). This means the switch fromoption.keytooption.valuein the selection handler is safe and maintains the expected behavior.Examples from the codebase:
ProviderConditionRender.tsx: Both key and value are set toprovider.idDeploymentConditionRender.tsx: Both key and value are set todeployment.idEnvironmentConditionRender.tsx: Both key and value are set toenvironment.idKindConditionRender.tsx: Both key and value are set tokind🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components that render ChoiceConditionRender rg -l "ChoiceConditionRender" | xargs rg -A 5 "onSelect"Length of output: 7323
Script:
#!/bin/bash # Check the type definition and usage of options in ChoiceConditionRender ast-grep --pattern 'options: { key: string; value: string; display: string }[]' # Check how options are constructed in parent components rg -A 5 "options\s*=\s*\[" apps/webservice/src/app/[workspaceSlug]/_components/Length of output: 165
Script:
#!/bin/bash # Check how options are passed to ChoiceConditionRender rg -B 5 -A 2 "options={" apps/webservice/src/app/[workspaceSlug]/_components/ # Check the construction of options array rg -B 3 -A 3 "const options" apps/webservice/src/app/[workspaceSlug]/_components/Length of output: 4666
packages/validators/src/releases/conditions/release-condition.ts (1)
5-5: LGTM! Import paths updated correctly.The changes to import paths for
VersionConditiontype andversionConditionconstant maintain consistency with the module structure and are used correctly throughout the file.Also applies to: 9-9
apps/webservice/src/app/[workspaceSlug]/(job)/jobs/JobTable.tsx (2)
1-30: LGTM! Well-organized imports and clean type definition.The imports are logically grouped and the component's props interface is clearly defined.
90-96: LGTM! Clear and user-friendly empty state handling.The empty state is well-handled with a clear way to reset filters.
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionBadge.tsx (3)
4-4: LGTM: Import changes align with condition type restructuring.The addition of
VersionConditionandDateOperatorimports from the conditions validator package reflects a good separation of concerns.Also applies to: 21-21
41-44: LGTM: Date operator mappings are complete and consistent.The date operator mappings maintain readability while properly utilizing the new
DateOperatorenum.
Line range hint
177-219: LGTM: Version condition handling is properly integrated.The version condition integration follows the established pattern and maintains type safety through proper type guards.
packages/db/src/schema/release.ts (3)
5-6: LGTM: Import restructuring improves type safety.The separation of operator types into specific
DateOperatorandVersionOperatorimports enhances type safety and makes the code more maintainable.Also applies to: 34-37
224-226: LGTM: Date operator handling is well-implemented.The date comparison logic correctly maps all
DateOperatorenum values to their corresponding SQL operations, maintaining type safety.
224-226: Verify integration with job filtering system.These changes to condition operators are part of the job filtering enhancement. Let's verify the integration with job-related components.
Also applies to: 232-234
✅ Verification successful
Let me analyze the results and gather more specific information about the integration between release and job components.
The initial results show that both job.ts and release.ts use these operators, but I need to verify how they're integrated and if the changes maintain consistency between them.
Based on the results, I can now provide a final response as we have sufficient evidence about the integration.
Changes are properly integrated with job filtering system
The modifications to date and version operators are consistently implemented across both release and job components:
- Both
job.tsandrelease.tsuse identical operator handling for date conditions (Before, After, BeforeOrOn)- Version condition handling is synchronized between both components
- The operator imports and usage patterns are consistent across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of these condition builders in job-related files # Test 1: Check for direct usage of these condition builders echo "Checking for direct usage of condition builders..." rg -l "buildCreatedAtCondition|buildVersionCondition" --type ts # Test 2: Check for usage of the new operator types echo "Checking for usage of new operator types..." rg -l "DateOperator\.|VersionOperator\." --type ts # Test 3: Check for any potential missed operator imports echo "Checking for potential missed operator imports..." rg -l "ReleaseOperator" --type tsLength of output: 1697
Script:
#!/bin/bash # Check the relationship between job and release condition handling # Look at how job.ts uses these conditions echo "=== Job condition handling ===" rg -A 10 "DateOperator|VersionOperator" packages/db/src/schema/job.ts # Check how release conditions are used in job context echo -e "\n=== Release-Job relationship ===" rg -A 5 "release" packages/db/src/schema/job.ts # Examine the job condition builders echo -e "\n=== Job condition builders ===" ast-grep --pattern 'function build$_Condition' packages/db/src/schema/job.tsLength of output: 2387
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (2)
6-11: LGTM: Import statements are well-organized.The new imports are properly structured and necessary for implementing the job filtering functionality.
Also applies to: 20-22
37-37: Verify filter integration with the API.Let's verify that the filter parameter is properly supported by the API endpoint and that the filter state is preserved across component re-renders.
Also applies to: 41-42
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Filter integration is properly implemented and supported
The verification shows that:
- The API endpoint
job.config.byReleaseIdaccepts and processes the filter parameter correctly- The filter state is preserved through URL parameters using the
useJobFilterhook- The component properly passes the filter to the API query and handles loading states
- The query is set up with proper refetch intervals and placeholder data for optimal UX
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the API endpoint properly handles the filter parameter rg -A 5 "byReleaseId" --glob "*.{ts,tsx}" # Check for filter state persistence implementation rg -A 10 "useJobFilter" --glob "*.{ts,tsx}"Length of output: 15359
packages/api/src/router/deployment.ts (1)
140-154: LGTM! Clean implementation of thebyIdmethod.The implementation follows best practices with proper:
- Input validation using zod
- Authorization checks
- Error handling via tRPC
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ComparisonConditionRender.tsx (2)
33-33: LGTM! Good architectural improvement.The addition of a dedicated
DateOperatorimport improves the separation of concerns by moving date-specific operators to a dedicated validator.
281-281: Verify consistent DateOperator usage across the codebase.The change from
ReleaseOperator.BeforetoDateOperator.Beforeimproves type safety for date operations. Let's verify this pattern is consistently applied.✅ Verification successful
DateOperator is consistently used across the codebase for date operations
The verification shows that
DateOperatoris consistently used across the codebase for date-related operations. There are no instances ofReleaseOperatorbeing used for date operations. TheDateOperatorenum is properly utilized in:
- Date condition validators (
packages/validators/src/conditions/date-condition.ts)- Database schemas for jobs and releases
- UI components for filters and condition rendering
- Condition badges for both jobs and releases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining ReleaseOperator usage with date operations # and verify DateOperator is consistently used for date handling # Search for potential inconsistencies in operator usage echo "Checking for potential mixed operator usage..." rg -A 2 "ReleaseOperator\.(Before|After)" # Verify DateOperator adoption echo "Verifying DateOperator usage in date-related components..." rg -A 2 "DateOperator\.(Before|After)"Length of output: 4998
packages/validators/src/jobs/conditions/job-condition.ts (1)
97-105: Confirm correct recursion depth handling indoesConvertingToComparisonRespectMaxDepthThe function
doesConvertingToComparisonRespectMaxDepthrecursively checks nested conditions to ensure they do not exceedMAX_DEPTH_ALLOWED. It's crucial to verify that the depth incrementation and base cases are properly implemented to prevent infinite recursion or incorrect evaluations.Please confirm that
depth + 1accurately represents the increasing depth and that the function correctly returnsfalsewhen the maximum depth is exceeded.packages/db/src/schema/job.ts (3)
1-6: Import Statements Are Correct and Well-OrganizedThe import statements for types and enums from
@ctrlplane/validatorsand other packages are correctly specified and enhance code clarity.Also applies to: 34-41
226-233: 'jobMatchesCondition' Function Effectively Builds SQL ConditionsThe
jobMatchesConditionfunction properly checks for the presence of conditions and delegates tobuildConditionwhen necessary. The logic is clear and aligns with expected behavior.
209-224: Validate Logical Operations Handling in 'buildCondition' FunctionThe
buildConditionfunction should correctly handle complex nested conditions involving logical operators. Ensure that the recursion and operator application (AND/ORwith optionalNOT) work as intended.Run the following script to confirm the logic:
#!/bin/bash # Description: Verify that 'buildCondition' correctly handles nested logical conditions. # Test: Search for unit tests covering complex conditions. rg -A 5 'describe.*buildCondition' tests/ # If unit tests are missing, consider adding tests for conditions like: # - Nested AND/OR combinations # - Conditions with 'not' flagsapps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx (2)
51-380: Overall Code QualityThe
JobComparisonConditionRendercomponent is well-structured and effectively manages complex state and user interactions related to job comparison conditions. The use of functional components and hooks aligns with React best practices.
154-165:⚠️ Potential issueCorrect the Condition for Rendering Operator Labels
It appears that the condition
{index !== 1 && (...)}might not correctly control the rendering of the operator labels. If the intention is to display operator labels ("And", "Or") for all items except the first one, consider changing the condition to{index !== 0 && (...)}.Apply this diff to fix the condition:
- {index !== 1 && ( + {index !== 0 && (Likely invalid or redundant comment.
packages/api/src/router/job.ts (7)
41-41: Confirm the Proper Import ofjobMatchesConditionAt line 41,
jobMatchesConditionis imported. Ensure that this function is correctly exported from"@ctrlplane/db/schema"and that there are no naming conflicts or deprecated uses.
61-61: Verify the Source ofJobStatusImportLine 61 imports
JobStatusfrom"@ctrlplane/validators/jobs". Confirm that this is the correct source forJobStatusand that it includes all necessary status values.
Line range hint
161-205: Ensure Proper Filtering withjobMatchesConditionThe query in
byWorkspaceIdusesjobMatchesConditionat line 166 to filter jobs. Verify that this condition correctly filters jobs based on the providedfilterinput.To confirm that
jobMatchesConditionworks as intended, you can:
- Review unit tests covering
jobMatchesCondition.- Ensure that the
filterparameter is properly validated and sanitized.Consider adding exhaustive tests if they are not already in place.
349-354: Verify Filtering Logic inbyReleaseIdQueryThe
whereclause in lines 349-354 includesjobMatchesCondition(ctx.db, input.filter). Confirm that this filtering behaves correctly and does not exclude or include unintended records.Ensure that:
- The
jobMatchesConditionfunction correctly interprets thefilterinput.- The function is performant and does not add significant overhead to the query.
- Adequate tests cover various filter scenarios.
610-633: Implement NewmetadataKeysRouterLines 610-633 introduce
metadataKeysRouter, providing a way to retrieve metadata keys byreleaseId. Verify that this new router is correctly integrated and that authorization checks are appropriately applied.To validate:
- Ensure that the route is accessible when expected and returns the correct data.
- Confirm that unauthorized access is properly restricted.
- Add unit and integration tests for this router.
Additionally, consider error handling for cases where the
releaseIddoes not exist or the user lacks permissions.
146-152:⚠️ Potential issueUpdate Procedure Input Schema in
byWorkspaceIdThe
byWorkspaceIdprocedure now expects an object withworkspaceId,filter,limit, andoffsetproperties. Ensure all calls to this procedure are updated to match the new input schema to prevent runtime errors.If any callers still pass a string UUID directly, they will need to be updated.
Run the following script to identify any outdated usages:
#!/bin/bash # Description: Find usages of 'byWorkspaceId' that need updating. rg 'byWorkspaceId\(\s*["\']?[a-f0-9-]{36}["\']?\s*\)'This command searches for calls to
byWorkspaceIdthat pass a single UUID rather than an object.
330-337:⚠️ Potential issueModify Input Schema in
byReleaseIdProcedureIn lines 332-337, the
byReleaseIdprocedure's input schema now accepts an object withreleaseIdand an optionalfilter. Ensure that all invocations of this procedure are updated to use the new input format to avoid breaking changes.Check for any calls like
api.releaseJobTrigger.byReleaseId(someReleaseId)and update them to:api.releaseJobTrigger.byReleaseId({ releaseId: someReleaseId, filter: /* optional filter */ });Use the following script to find outdated usages:
✅ Verification successful
The previous script had a syntax error. Let's try with a broader search pattern to find any usage of
byReleaseIdand also look at its procedure definition for better context.
Let's narrow down our search to specifically look for
releaseJobTrigger.byReleaseIdcalls since that's what the review comment is concerned about, and also look at the procedure definition in the job router for more context.
No Breaking Changes Found in
byReleaseIdUsageBased on the search results, all existing calls to
byReleaseIdare already using the correct object parameter format withreleaseIdand optionalfilter. The calls are found in:
JobsTable.tsx:{ releaseId }FlowNode.tsx:{ releaseId: data.release.id }TargetReleaseTable.tsx:{ releaseId: release.id, filter }No outdated usage patterns were found that need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Identify calls to 'byReleaseId' that need updating. rg 'byReleaseId\(\s*["\']?[a-f0-9-]{36}["\']?\s*\)'Length of output: 93
Script:
#!/bin/bash # Let's search for any calls to byReleaseId with a more flexible pattern rg "byReleaseId\(" -A 2 # Also search for the procedure definition to understand its usage ast-grep --pattern 'procedure($_, "byReleaseId")'Length of output: 566
Script:
#!/bin/bash # Let's search for any calls to byReleaseId specifically from releaseJobTrigger rg "releaseJobTrigger.*byReleaseId" -A 2 # Also search for the procedure definition in job.ts rg "byReleaseId.*procedure" packages/api/src/router/job.ts -A 5Length of output: 1690
| Equals = "equals", | ||
| } | ||
|
|
||
| export type VersionOperatorType = "like" | "regex" | "equals"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider deriving VersionOperatorType from the zod schema.
To maintain DRY principle and ensure type consistency, consider deriving the type from the zod schema:
-export type VersionOperatorType = "like" | "regex" | "equals";
+export type VersionOperatorType = z.infer<typeof versionCondition>["operator"];Committable suggestion was skipped due to low confidence.
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/VersionConditionRender.tsx
Show resolved
Hide resolved
.../webservice/src/app/[workspaceSlug]/_components/job-condition/JobMetadataConditionRender.tsx
Show resolved
Hide resolved
| const convertToComparison = (index: number) => { | ||
| const cond = condition.conditions[index]; | ||
| if (!cond) return; | ||
|
|
||
| const newComparisonCondition: ComparisonCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.And, | ||
| conditions: [cond], | ||
| }; | ||
|
|
||
| const newCondition = { | ||
| ...condition, | ||
| conditions: condition.conditions.map((c, i) => | ||
| i === index ? newComparisonCondition : c, | ||
| ), | ||
| }; | ||
| onChange(newCondition); | ||
| }; |
There was a problem hiding this comment.
Handle Edge Cases in convertToComparison Function
The convertToComparison function does not account for scenarios where converting the condition might exceed the maximum allowed depth. Ensure that this function respects the maximum depth constraint to prevent potential runtime errors.
Apply this diff to include depth checking:
const convertToComparison = (index: number) => {
const cond = condition.conditions[index];
if (!cond) return;
+ if (!doesConvertingToComparisonRespectMaxDepth(depth + 1, cond)) {
+ // Optionally, provide feedback to the user about the depth limit
+ return;
+ }
const newComparisonCondition: ComparisonCondition = {
type: FilterType.Comparison,
operator: ComparisonOperator.And,
conditions: [cond],
};
const newCondition = {
...condition,
conditions: condition.conditions.map((c, i) =>
i === index ? newComparisonCondition : c,
),
};
onChange(newCondition);
};Committable suggestion was skipped due to low confidence.
...ebservice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx
Outdated
Show resolved
Hide resolved
packages/api/src/router/job.ts
Outdated
| variables: _.chain(v) | ||
| .filter((v) => isPresent(v.job_variable)) | ||
| .groupBy((v) => v.job_variable!.id) | ||
| .map((v) => v[0]!.job_variable!) | ||
| .value(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Variables Aggregation Logic
Similarly, the aggregation of variables can be streamlined for efficiency.
Consider the following refactor:
variables: _.chain(v)
- .filter((v) => isPresent(v.job_variable))
- .groupBy((v) => v.job_variable!.id)
- .map((v) => v[0]!.job_variable!)
+ .map((row) => row.job_variable)
+ .filter(isPresent)
+ .uniqBy('id')
.value(),📝 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.
| variables: _.chain(v) | |
| .filter((v) => isPresent(v.job_variable)) | |
| .groupBy((v) => v.job_variable!.id) | |
| .map((v) => v[0]!.job_variable!) | |
| .value(), | |
| variables: _.chain(v) | |
| .map((row) => row.job_variable) | |
| .filter(isPresent) | |
| .uniqBy('id') | |
| .value(), |
packages/api/src/router/job.ts
Outdated
| metadata: _.chain(v) | ||
| .filter((v) => isPresent(v.job_metadata)) | ||
| .groupBy((v) => v.job_metadata!.id) | ||
| .map((v) => v[0]!.job_metadata!) | ||
| .value(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Metadata Aggregation Logic
The Lodash chain used to process metadata can be simplified for better performance and readability.
Consider simplifying the chain as follows:
metadata: _.chain(v)
- .filter((v) => isPresent(v.job_metadata))
- .groupBy((v) => v.job_metadata!.id)
- .map((v) => v[0]!.job_metadata!)
+ .map((row) => row.job_metadata)
+ .filter(isPresent)
+ .uniqBy('id')
.value(),📝 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.
| metadata: _.chain(v) | |
| .filter((v) => isPresent(v.job_metadata)) | |
| .groupBy((v) => v.job_metadata!.id) | |
| .map((v) => v[0]!.job_metadata!) | |
| .value(), | |
| metadata: _.chain(v) | |
| .map((row) => row.job_metadata) | |
| .filter(isPresent) | |
| .uniqBy('id') | |
| .value(), |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (2)
58-62: Consider adding type safety for approvals data.While the current implementation works, it could benefit from explicit type annotations for better type safety.
- const approvals = approvalsQ.data ?? []; + const approvals: typeof approvalsQ.data = approvalsQ.data ?? [];
223-234: Enhance table accessibility.Consider adding ARIA attributes to improve table accessibility.
- <Table className="table-fixed"> + <Table + className="table-fixed" + role="grid" + aria-label="Job releases table" + >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (5 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/page.tsx (0 hunks)
- packages/api/src/router/deployment.ts (1 hunks)
- packages/api/src/router/environment.ts (1 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx
🔇 Additional comments (7)
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (4)
11-11: LGTM: Clean import organization.The new imports are well-organized and follow the existing pattern.
Also applies to: 22-24
46-46: Previous issue still needs to be addressed: Add error handling for links metadata parsing.The previous review comment about adding error handling for JSON parsing is still valid and hasn't been addressed.
Also applies to: 52-53
213-221: Add keyboard accessibility to the filter dialog button.The filter dialog button should be keyboard accessible for better usability.
<Button variant="ghost" size="icon" className="h-7 w-7" + aria-label="Filter jobs" + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + // trigger dialog open + } + }} >
46-46: Verify filter implementation impact.Let's verify that the filter implementation properly integrates with the API and doesn't cause unnecessary re-renders.
Also applies to: 52-53
✅ Verification successful
Based on the search results, I can now provide a final response since we have found the implementation of
useJobFilterand its usage in the TargetReleaseTable component.Filter implementation is properly integrated and safe to use.
The
useJobFilterhook is correctly implemented with:
- URL-based state management using
useSearchParams- Proper error handling for JSON parsing
- Memoization to prevent unnecessary re-renders
- Integration with the router for state updates
The hook is used consistently across components including TargetReleaseTable and JobTable, with proper destructuring of both
filterandsetFiltervalues where needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper filter usage across the codebase echo "Checking filter implementation..." # Look for filter usage in API routes echo "Checking API routes for filter handling:" rg -A 5 "byReleaseId.*filter" --type ts # Check for proper error handling in filter-related code echo "Checking filter error handling:" rg -A 5 "useJobFilter.*error" --type tsLength of output: 324
Script:
#!/bin/bash # Let's try a more comprehensive search for filter-related code # Check for useJobFilter hook implementation echo "Checking useJobFilter implementation:" ast-grep --pattern 'export function useJobFilter() { $$$ }' # Look for filter usage in the component echo "Checking filter usage in TargetReleaseTable:" rg -l "TargetReleaseTable" --type tsx | xargs cat # Check for filter-related types and interfaces echo "Checking filter-related types:" rg -A 3 "type.*Filter|interface.*Filter" --type ts # Look for API calls using filter echo "Checking API calls with filter:" rg -A 5 "useQuery.*filter" --type tsxLength of output: 37492
Script:
#!/bin/bash # Let's check for the useJobFilter hook implementation and its usage # Search for useJobFilter hook definition echo "Checking useJobFilter hook definition:" rg -A 10 "export.*useJobFilter" --type ts --type tsx # Search for filter usage in TargetReleaseTable echo "Checking TargetReleaseTable implementation:" rg -A 10 "TargetReleaseTable.*filter" --type ts --type tsx # Look for job filter related hooks and components echo "Checking job filter related components:" rg -A 5 "useJobFilter|JobFilter" --type ts --type tsxLength of output: 528
Script:
#!/bin/bash # Let's try without specifying file types since the environment might handle extensions differently # Search for useJobFilter hook definition echo "Checking useJobFilter hook definition:" rg -A 10 "export.*useJobFilter" # Search for filter usage in TargetReleaseTable echo "Checking TargetReleaseTable implementation:" rg -A 10 "TargetReleaseTable.*filter" # Look for job filter related hooks and components echo "Checking job filter related components:" rg -A 5 "useJobFilter" # Check for filter-related imports in the component echo "Checking filter imports:" rg "import.*filter" "apps/webservice/src/app/\[workspaceSlug\]/systems/\[systemSlug\]/deployments/\[deploymentSlug\]/releases/\[versionId\]/TargetReleaseTable"Length of output: 7442
packages/api/src/router/deployment.ts (1)
144-158: 🛠️ Refactor suggestionInclude related entities in the response for consistency.
The
byIdimplementation should follow the pattern established by other methods in this router (bySlug,bySystemId) by including related entities (latest release, system, workspace) in the response. This would:
- Maintain consistency across the API
- Potentially reduce the number of subsequent API calls needed by clients
- Improve performance by fetching related data in a single query
Reusing the implementation suggestion from the previous review:
byId: protectedProcedure .input(z.string().uuid()) .meta({ authorizationCheck: ({ canUser, input }) => canUser .perform(Permission.DeploymentGet) .on({ type: "deployment", id: input }), }) .query(({ ctx, input }) => { + const latestRelease = latestActiveReleaseSubQuery(ctx.db); return ctx.db .select() .from(deployment) + .innerJoin(system, eq(system.id, deployment.systemId)) + .innerJoin(workspace, eq(system.workspaceId, workspace.id)) + .leftJoin( + latestRelease, + and( + eq(latestRelease.deploymentId, deployment.id), + eq(latestRelease.rank, 1), + ), + ) .where(eq(deployment.id, input)) - .then(takeFirst), + .then(takeFirst) + .then((r) => ({ + ...r.deployment, + system: { ...r.system, workspace: r.workspace }, + latestRelease: r.release, + })), }),packages/api/src/router/environment.ts (2)
500-518:⚠️ Potential issueAdd filter for deleted environments and pagination support.
The implementation needs the following improvements:
- Filter out deleted environments to maintain consistency with other procedures like
bySystemId.- Add pagination support for better performance with large datasets.
Apply this diff to add both improvements:
- .input(z.string().uuid()) + .input(z.object({ + workspaceId: z.string().uuid(), + limit: z.number().min(1).max(100).default(50), + offset: z.number().min(0).default(0) + })) .query(({ ctx, input }) => ctx.db .select() .from(environment) .innerJoin(system, eq(environment.systemId, system.id)) - .where(eq(system.workspaceId, input)) + .where( + and( + eq(system.workspaceId, input.workspaceId), + isNull(environment.deletedAt) + ) + ) .orderBy(environment.name) + .limit(input.limit) + .offset(input.offset) .then((envs) => envs.map((e) => ({ ...e.environment, system: e.system })), ),
500-518:⚠️ Potential issueInclude target information for API consistency.
The
bySystemIdprocedure includes target information for environments, but this procedure doesn't maintain this consistency. This inconsistency could lead to issues when switching between these related procedures.Apply this diff to include target information:
.then((envs) => - envs.map((e) => ({ ...e.environment, system: e.system })), + Promise.all( + envs.map(async (e) => ({ + ...e.environment, + system: e.system, + targets: + e.environment.targetFilter != null + ? await ctx.db + .select() + .from(target) + .where( + targetMatchesMetadata(ctx.db, e.environment.targetFilter) + ) + : [], + })), + ), ),
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (5)
packages/validators/src/jobs/conditions/job-target-condition.ts (2)
3-7: Consider enhancing developer experience with custom error messages.The schema could benefit from custom error messages to provide better feedback when validation fails.
export const jobTargetCondition = z.object({ - type: z.literal("target"), - operator: z.literal("equals"), - value: z.string().uuid(), + type: z.literal("target", { + invalid_type_error: "Job target condition type must be 'target'", + }), + operator: z.literal("equals", { + invalid_type_error: "Job target condition operator must be 'equals'", + }), + value: z.string().uuid({ + message: "Job target condition value must be a valid UUID", + }), });
1-9: Consider future extensibility and documentation.A few architectural suggestions:
- Consider supporting additional operators (e.g., 'contains', 'startsWith') if there's a future need
- Add JSDoc comments explaining the purpose and usage of this condition type
- Consider adding integration tests to verify the schema works correctly with the broader job condition system
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx (2)
53-54: Remove commented code.These commented lines appear to be unused. If they're no longer needed, they should be removed to maintain code cleanliness.
- // const workspaceTargetsFilter: TargetCondition | undefined = - // search != "" ? searchFilter : undefined;
95-106: Enhance accessibility with descriptive aria-label.The button could benefit from a more descriptive aria-label for better screen reader support.
<Button variant="outline" role="combobox" aria-expanded={open} + aria-label="Select target for job condition" className="w-full items-center justify-start gap-2 rounded-l-none rounded-r-md bg-transparent px-2 hover:bg-neutral-800/50" >apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionBadge.tsx (1)
163-163: Ensure all operator verbs are defined in the operatorVerbs mappingThe fallback operator
"equals"used here may not have a corresponding entry in theoperatorVerbsmapping. To prevent undefined values, consider adding a default case or ensuring all possible operators are included.Apply this diff to add the "equals" operator to
operatorVerbs:const operatorVerbs = { [ComparisonOperator.And]: "and", [ComparisonOperator.Or]: "or", [MetadataOperator.Equals]: "is", + equals: "is", [MetadataOperator.Null]: ( <span> is <span className="text-orange-500">null</span> </span> ), // ...rest of the operators };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/EnvironmentConditionRender.tsx (1 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/JobConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx (1 hunks)
- packages/api/src/router/environment.ts (2 hunks)
- packages/api/src/router/job.ts (10 hunks)
- packages/db/src/schema/job.ts (3 hunks)
- packages/validators/src/jobs/conditions/comparison-condition.ts (1 hunks)
- packages/validators/src/jobs/conditions/index.ts (1 hunks)
- packages/validators/src/jobs/conditions/job-condition.ts (1 hunks)
- packages/validators/src/jobs/conditions/job-target-condition.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionRender.tsx
- packages/validators/src/jobs/conditions/comparison-condition.ts
- packages/validators/src/jobs/conditions/index.ts
🔇 Additional comments (25)
packages/validators/src/jobs/conditions/job-target-condition.ts (3)
1-1: LGTM!The Zod import is correctly defined and appropriately scoped.
3-7: LGTM! Schema structure is well-defined.The schema correctly validates the structure of a job target condition with appropriate type restrictions and UUID validation.
9-9: LGTM! Type export is correctly defined.The type inference from the Zod schema ensures type safety and schema validation remain synchronized.
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx (1)
30-35: LGTM! Clean component definition with proper TypeScript typing.The component is well-structured with appropriate state management for search and popover control.
packages/db/src/schema/job.ts (3)
Line range hint
1-47: LGTM: Well-organized importsThe imports are logically grouped and all imported types and operators are utilized in the implementation.
140-191: Refer to the previous review comment about refactoringThe previous review comment about refactoring the
buildMetadataConditionfunction to reduce repetition is still applicable.
202-208: Refer to the previous review comment about regex validationThe previous review comment about adding regex pattern validation and sanitization is still applicable.
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx (5)
1-50: LGTM! Well-organized imports and type definitions.The imports are properly organized and grouped by their purposes (types, UI components, icons). All imported entities appear to be used in the component.
78-95:⚠️ Potential issueAdd depth validation to prevent runtime errors.
The
convertToComparisonfunction should validate depth constraints before proceeding with the conversion, as noted in the past review.
151-193:⚠️ Potential issueUse stable and unique keys for list items.
Using array indices as keys can lead to rendering issues when items are reordered or removed.
224-248: 🛠️ Refactor suggestionSimplify conditional rendering in dropdown menu.
The complex conditions for menu items can be simplified for better readability.
276-338: Consider validating default values for new conditions.The default values for new conditions (empty strings, current date) might benefit from validation or normalization.
Let's verify the validation requirements for these fields:
packages/api/src/router/environment.ts (2)
458-468: LGTM! System information correctly included.The changes properly integrate system information into the environment data, maintaining consistency with other procedures.
505-523:⚠️ Potential issueAdd missing filters and pagination support.
The procedure has several areas for improvement:
- Missing filter for soft-deleted environments
- No pagination support for large workspaces
- Inconsistent with
bySystemIdwhich includes target informationApply this diff to address the issues:
.input(z.string().uuid()) + .input(z.object({ + workspaceId: z.string().uuid(), + limit: z.number().min(1).max(100).default(50), + offset: z.number().min(0).default(0) + })) .query(({ ctx, input }) => ctx.db .select() .from(environment) .innerJoin(system, eq(environment.systemId, system.id)) - .where(eq(system.workspaceId, input)) + .where( + and( + eq(system.workspaceId, input.workspaceId), + isNull(environment.deletedAt) + ) + ) .orderBy(environment.name) + .limit(input.limit) + .offset(input.offset) .then((envs) => - envs.map((e) => ({ ...e.environment, system: e.system })), + Promise.all( + envs.map(async (e) => ({ + ...e.environment, + system: e.system, + targets: + e.environment.targetFilter != null + ? await ctx.db + .select() + .from(target) + .where( + targetMatchesMetadata(ctx.db, e.environment.targetFilter) + ) + : [], + })) + ) ),Let's verify if the workspace exists before querying:
✅ Verification successful
Let me verify if the workspace existence check is handled by the authorization check in the procedure.
Add missing filters and pagination support.
The procedure needs improvements for better data handling and consistency:
- Add filter for soft-deleted environments
- Add pagination support for large workspaces
- Include target information for consistency with
bySystemIdThe workspace existence is implicitly verified through the authorization check using
canUser.perform(Permission.SystemGet).on({ type: "workspace", id: input }), so no additional verification is needed.Apply this diff to address the issues:
.input(z.string().uuid()) + .input(z.object({ + workspaceId: z.string().uuid(), + limit: z.number().min(1).max(100).default(50), + offset: z.number().min(0).default(0) + })) .query(({ ctx, input }) => ctx.db .select() .from(environment) .innerJoin(system, eq(environment.systemId, system.id)) - .where(eq(system.workspaceId, input)) + .where( + and( + eq(system.workspaceId, input.workspaceId), + isNull(environment.deletedAt) + ) + ) .orderBy(environment.name) + .limit(input.limit) + .offset(input.offset) .then((envs) => - envs.map((e) => ({ ...e.environment, system: e.system })), + Promise.all( + envs.map(async (e) => ({ + ...e.environment, + system: e.system, + targets: + e.environment.targetFilter != null + ? await ctx.db + .select() + .from(target) + .where( + targetMatchesMetadata(ctx.db, e.environment.targetFilter) + ) + : [], + })) + ) ),🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if workspace existence is verified elsewhere # Look for workspace existence checks in router files rg -A 5 "workspaceId.*input" --type tsLength of output: 8130
Script:
#!/bin/bash # Check if the authorization check implicitly verifies workspace existence ast-grep --pattern 'canUser.perform($_).on({ type: "workspace", id: $_ })'Length of output: 9121
packages/api/src/router/job.ts (4)
146-158: LGTM: Input type changes improve API flexibilityThe changes to input types for both procedures enhance the API by:
- Adding optional job condition filtering
- Including proper pagination controls with reasonable limits
- Using strong type validation with zod
Also applies to: 333-338
164-167: LGTM: Query filtering implementation is consistent and secureThe filtering implementation:
- Consistently uses jobMatchesCondition across procedures
- Properly maintains existing security constraints using 'and'
Also applies to: 350-355
183-204: Optimize the count query performanceThe count query duplicates many joins from the main query, which could impact performance. Consider optimizing it:
-const total = await ctx.db - .select({ - count: countDistinct(releaseJobTrigger.id), - }) - .from(releaseJobTrigger) - .innerJoin(job, eq(releaseJobTrigger.jobId, job.id)) - .innerJoin(target, eq(releaseJobTrigger.targetId, target.id)) - .innerJoin(release, eq(releaseJobTrigger.releaseId, release.id)) - .innerJoin(deployment, eq(release.deploymentId, deployment.id)) - .innerJoin(environment, eq(releaseJobTrigger.environmentId, environment.id)) - .innerJoin(system, eq(environment.systemId, system.id)) +const total = await ctx.db + .select({ + count: countDistinct(releaseJobTrigger.id), + }) + .from(releaseJobTrigger) + .innerJoin(job, eq(releaseJobTrigger.jobId, job.id)) + .innerJoin(environment, eq(releaseJobTrigger.environmentId, environment.id)) + .innerJoin(system, eq(environment.systemId, system.id)) + .where( + and( + eq(system.workspaceId, input.workspaceId), + jobMatchesCondition(ctx.db, input.filter), + ), + )
611-633: LGTM: Well-implemented metadata keys routerThe new metadataKeys router is:
- Efficiently implemented using selectDistinct
- Properly secured with authorization checks
- Correctly joined to fetch release-specific metadata keys
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/EnvironmentConditionRender.tsx (1)
1-72: Overall, the code is well-structured and follows best practicesThe implementation is clear, and the use of React hooks and TRPC queries is appropriate.
packages/validators/src/jobs/conditions/job-condition.ts (6)
28-36: LGTM: ComprehensiveJobConditionType UnionThe
JobConditiontype effectively unifies all condition types, enhancing type safety and flexibility in job condition handling.
38-47: LGTM: Accurate Zod Schema forjobConditionThe
jobConditionschema correctly constructs a union of all condition schemas, ensuring comprehensive validation across different condition types.
49-54: LGTM: Sensible Default for Job ConditionsThe
defaultConditionprovides a logical starting point with an empty comparison condition, facilitating consistent initialization.
63-65: LGTM: Correct Implementation ofisEmptyConditionThe
isEmptyConditionfunction accurately identifies empty comparison conditions, which is important for validation and logic flow.
96-100: LGTM: Proper Type Guard forisJobTargetConditionThe
isJobTargetConditionfunction correctly identifiesJobTargetConditiontypes, ensuring proper type narrowing in the codebase.
102-115: LGTM: Effective Depth Checking indoesConvertingToComparisonRespectMaxDepthThe function accurately checks the maximum depth when converting to a comparison condition, preventing potential stack overflows and ensuring compliance with
MAX_DEPTH_ALLOWED.
| onSelect={() => { | ||
| onChange({ ...condition, value: target.id }); | ||
| setOpen(false); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider debouncing the search input.
The search input might trigger too many API calls as the user types. Consider implementing debouncing for better performance.
import { useDebouncedCallback } from 'use-debounce';
// Inside component:
const debouncedSetSearch = useDebouncedCallback(
(value: string) => setSearch(value),
300
);
// In CommandInput:
onValueChange={debouncedSetSearch}
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx
Show resolved
Hide resolved
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx
Show resolved
Hide resolved
...ebservice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx
Show resolved
Hide resolved
| const options = environments.map((environment) => ({ | ||
| key: environment.id, | ||
| value: environment.id, | ||
| display: `${environment.name} (${environment.system.name})`, | ||
| })); |
There was a problem hiding this comment.
Prevent potential runtime errors when accessing environment.system.name
In the options and selectedDisplay constructions, environment.system may be undefined, leading to runtime errors when accessing environment.system.name. Ensure that environment.system is defined before accessing its name property.
Apply this diff to fix the issue:
const options = environments.map((environment) => ({
key: environment.id,
value: environment.id,
- display: `${environment.name} (${environment.system.name})`,
+ display: environment.system
+ ? `${environment.name} (${environment.system.name})`
+ : environment.name,
}));
const selectedDisplay = selectedEnvironment
- ? `${selectedEnvironment.name} (${selectedEnvironment.system.name})`
+ ? selectedEnvironment.system
+ ? `${selectedEnvironment.name} (${selectedEnvironment.system.name})`
+ : selectedEnvironment.name
: null;Also applies to: 58-60
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionBadge.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseVersionConditionRender.tsx (1)
17-25: Consider adding specific className type.While the implementation is correct, the className prop could benefit from more specific typing.
Consider updating the ReleaseConditionRenderProps type to specify className as string:
interface ReleaseConditionRenderProps<T> { condition: T; onChange: (condition: T) => void; className?: string; }apps/webservice/src/app/[workspaceSlug]/_components/filter/DateConditionRender.tsx (1)
Line range hint
13-31: Consider moving the date utility function to a shared location.The
toZonedDateTimeutility function could be moved to a shared utilities file since it's a generic date conversion function that might be useful in other components.Consider creating a new file
utils/date.ts:import { ZonedDateTime } from "@internationalized/date"; import ms from "ms"; export const toZonedDateTime = (date: Date): ZonedDateTime => { const timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone; const offset = -date.getTimezoneOffset() * ms("1m"); const year = date.getFullYear(); const month = date.getMonth() + 1; const day = date.getDate(); const hour = date.getHours(); const minute = date.getMinutes(); const second = date.getSeconds(); const millisecond = date.getMilliseconds(); return new ZonedDateTime( year, month, day, timeZone, offset, hour, minute, second, millisecond, ); };apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (1)
61-69: Consider consistent message formattingWhile the loading state implementation is good, consider making the loading message consistent with the empty state message format.
- Loading {type}s... + No {type}s to show yetapps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx (1)
96-109: Enhance accessibility with ARIA labels.Consider adding more descriptive ARIA labels to improve accessibility.
<Button variant="outline" role="combobox" aria-expanded={open} + aria-label="Select target" + aria-description={target?.name ?? "No target selected"} className="w-full items-center justify-start gap-2 rounded-l-none rounded-r-md bg-transparent px-2 hover:bg-neutral-800/50" >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (6 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/DateConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/VersionConditionRender.tsx (1 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/JobConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/job-condition-props.ts (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionRender.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseVersionConditionRender.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (2 hunks)
- packages/api/src/router/job.ts (10 hunks)
- packages/validators/src/conditions/version-condition.ts (1 hunks)
- packages/validators/src/jobs/index.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/webservice/src/app/[workspaceSlug]/_components/filter/VersionConditionRender.tsx
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobComparisonConditionRender.tsx
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionRender.tsx
- apps/webservice/src/app/[workspaceSlug]/_components/job-condition/job-condition-props.ts
- apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionRender.tsx
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx
- packages/validators/src/conditions/version-condition.ts
- packages/validators/src/jobs/index.ts
🔇 Additional comments (18)
apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseVersionConditionRender.tsx (3)
1-9: LGTM! Clean and well-organized imports.The imports are properly structured with external dependencies first, followed by internal imports. All necessary types and components are correctly imported.
10-12: LGTM! Well-typed component declaration.The component is properly typed as a React FC with appropriate generic constraints, ensuring type safety for version condition props.
1-26: Verify integration with JobTable component.Since this component is part of the job filtering system, let's verify its integration with the JobTable component.
✅ Verification successful
Let me gather more information about the component hierarchy and integration.
Based on the gathered information, I can now generate the final response:
Component is correctly integrated within the filtering system
The verification shows that:
ReleaseVersionConditionRenderis properly integrated into theReleaseConditionRenderparent component- It's part of a well-structured condition rendering system that handles different types of conditions (version, metadata, created_at, etc.)
- The component is indirectly connected to the JobTable through the
JobConditionDialogandJobConditionBadgecomponents, which use the condition system for filtering🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of ReleaseVersionConditionRender in JobTable # and other related components # Search for imports of ReleaseVersionConditionRender echo "Checking imports of ReleaseVersionConditionRender:" rg "import.*ReleaseVersionConditionRender" -A 3 # Search for usage of ReleaseVersionConditionRender in JSX echo "Checking usage in JSX:" rg "<ReleaseVersionConditionRender" -A 5Length of output: 2042
Script:
#!/bin/bash # Check the implementation of ReleaseConditionRender and JobConditionRender echo "Checking ReleaseConditionRender implementation:" cat apps/webservice/src/app/[workspaceSlug]/_components/release-condition/ReleaseConditionRender.tsx echo -e "\nChecking JobConditionRender implementation:" cat apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionRender.tsx # Find where these condition components are used in the broader system echo -e "\nChecking usage of condition components in the system:" rg -l "JobConditionRender|ReleaseConditionRender" --type tsx # Check for JobTable integration echo -e "\nChecking JobTable component for condition usage:" fd "JobTable" --type f --exec rg "Condition" {} -A 3Length of output: 6466
apps/webservice/src/app/[workspaceSlug]/_components/filter/DateConditionRender.tsx (2)
62-62: LGTM! Good UI enhancement.Adding the truncate class is a good improvement to handle long type names gracefully.
Line range hint
89-95: Add error handling for invalid date values.The
new Date(value)conversion could throw ifvalueis invalid. Consider adding error handling or validation.Here's a suggested improvement:
<DateTimePicker - value={toZonedDateTime(new Date(value))} + value={(() => { + try { + return toZonedDateTime(new Date(value)); + } catch (e) { + console.error('Invalid date value:', value); + return toZonedDateTime(new Date()); + } + })()} onChange={setDate} aria-label={type} variant="filter" />apps/webservice/src/app/[workspaceSlug]/_components/filter/ChoiceConditionRender.tsx (4)
21-21: LGTM: Clean type definition for loading propThe optional loading prop is well-defined and follows TypeScript best practices.
30-30: LGTM: Proper default value for loading propThe default value is appropriately set and well-placed in the props destructuring.
39-39: LGTM: Improved filter condition readabilityThe addition of "is" makes the filter condition read more naturally while maintaining proper text capitalization.
76-78: Verify the impact of switching from key to valueThe change from
option.keytooption.valueinonSelectmight affect existing code. Please ensure all consumers of this component are updated accordingly.✅ Verification successful
Let me gather more information about how the component is used and what data is being passed to it.
Looking at the options data structure from the components, I can now generate the final response since I have enough information.
Change from
option.keytooption.valueis safe to proceedThe change is safe because in all consuming components, the
keyandvalueproperties are set to the same value:
- In
StatusConditionRender: Both come from object entries mapping- In
ProviderConditionRender: Both are set toprovider.id- In
KindConditionRender: Both are set tokind🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct usage of this component to verify the change impact rg -l "ChoiceConditionRender" | xargs rg "onSelect"Length of output: 1459
Script:
#!/bin/bash # Check the implementation of components using ChoiceConditionRender echo "=== Provider Condition ===" ast-grep --pattern 'const ProviderConditionRender = $_' -f apps/webservice/src/app/[workspaceSlug]/_components/target-condition/ProviderConditionRender.tsx echo -e "\n=== Kind Condition ===" ast-grep --pattern 'const KindConditionRender = $_' -f apps/webservice/src/app/[workspaceSlug]/_components/target-condition/KindConditionRender.tsx echo -e "\n=== Environment Condition ===" ast-grep --pattern 'const EnvironmentConditionRender = $_' -f apps/webservice/src/app/[workspaceSlug]/_components/job-condition/EnvironmentConditionRender.tsx echo -e "\n=== Status Condition ===" ast-grep --pattern 'const StatusConditionRender = $_' -f apps/webservice/src/app/[workspaceSlug]/_components/job-condition/StatusConditionRender.tsx # Also check the options being passed echo -e "\n=== Options Data Structure ===" rg -A 5 "options.*=.*\[" apps/webservice/src/app/[workspaceSlug]/_components/Length of output: 3853
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx (4)
1-30: LGTM! Well-organized imports.The imports are properly structured and all dependencies are clearly defined.
31-38: LGTM! Clean state management with debouncing.The component properly implements state management with debouncing for search optimization.
Note: A previous review comment suggested implementing debouncing, but this is already implemented correctly.
84-87: Add error handling for the targets query.The query doesn't handle error states, which could lead to a poor user experience if the API call fails.
91-95: Consider simplifying the grid layout.The current grid layout with 12 columns might be overengineered for a simple two-part input.
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionBadge.tsx (2)
81-87: Depth-based styling is still duplicatedThe depth-based color styling logic remains duplicated. Please refer to the previous review comment for the suggested utility function implementation.
Also applies to: 112-117
202-214: Data fetching components still lack proper loading/error handlingThe components fetching data for Deployment, Environment, and JobTarget conditions still need loading and error state handling. Please refer to the previous review comments for the suggested implementations.
Also applies to: 219-232, 249-261
packages/api/src/router/job.ts (3)
101-111: LGTM! Optimized metadata and variables processing.The refactored lodash chains are more efficient and readable, following the previous review suggestions.
333-338: LGTM! Well-structured input validation and filtering.The input schema and authorization checks are properly implemented.
611-633: Consider performance optimizations for metadata keys retrieval.While the implementation is correct, consider the following optimizations:
- Add an index on
jobMetadata.keyto improve query performance- Implement caching for the metadata keys as they don't change frequently
| const setOperator = (operator: VersionOperatorType) => | ||
| onChange({ ...condition, operator }); | ||
| const setValue = (value: string) => onChange({ ...condition, value }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing callback functions.
While the functions are correctly implemented, they could benefit from memoization to prevent unnecessary re-renders in child components.
Consider using useCallback:
- const setOperator = (operator: VersionOperatorType) =>
- onChange({ ...condition, operator });
- const setValue = (value: string) => onChange({ ...condition, value });
+ const setOperator = React.useCallback(
+ (operator: VersionOperatorType) => onChange({ ...condition, operator }),
+ [condition, onChange]
+ );
+ const setValue = React.useCallback(
+ (value: string) => onChange({ ...condition, value }),
+ [condition, onChange]
+ );📝 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: VersionOperatorType) => | |
| onChange({ ...condition, operator }); | |
| const setValue = (value: string) => onChange({ ...condition, value }); | |
| const setOperator = React.useCallback( | |
| (operator: VersionOperatorType) => onChange({ ...condition, operator }), | |
| [condition, onChange] | |
| ); | |
| const setValue = React.useCallback( | |
| (value: string) => onChange({ ...condition, value }), | |
| [condition, onChange] | |
| ); |
| const searchFilter: TargetCondition = { | ||
| type: TargetFilterType.Name, | ||
| operator: TargetOperator.Like, | ||
| value: `%${searchDebounced}%`, | ||
| }; | ||
|
|
||
| // const workspaceTargetsFilter: TargetCondition | undefined = | ||
| // search != "" ? searchFilter : undefined; | ||
|
|
||
| const systemQ = api.system.bySlug.useQuery( | ||
| { workspaceSlug, systemSlug: systemSlug ?? "" }, | ||
| { enabled: systemSlug != null }, | ||
| ); | ||
| const system = systemQ.data; | ||
| const envFilters = | ||
| system?.environments.map((env) => env.targetFilter).filter(isPresent) ?? []; | ||
|
|
||
| const systemFilter: TargetCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.Or, | ||
| conditions: envFilters, | ||
| }; | ||
|
|
||
| const systemTargetsFilter: TargetCondition | undefined = | ||
| system != null | ||
| ? { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.And, | ||
| conditions: [searchFilter, systemFilter], | ||
| } | ||
| : undefined; | ||
|
|
||
| const filter = systemTargetsFilter ?? searchFilter; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing filter construction.
The filter construction logic could be simplified by extracting it into a separate function for better maintainability.
const buildTargetFilter = (
searchFilter: TargetCondition,
system: typeof systemQ.data
): TargetCondition => {
if (!system) return searchFilter;
const systemFilter: TargetCondition = {
type: FilterType.Comparison,
operator: ComparisonOperator.Or,
conditions: system.environments
.map((env) => env.targetFilter)
.filter(isPresent),
};
return {
type: FilterType.Comparison,
operator: ComparisonOperator.And,
conditions: [searchFilter, systemFilter],
};
};
// Usage
const filter = buildTargetFilter(searchFilter, system);| <div className="cursor-pointer rounded-lg bg-inherit text-xs text-muted-foreground"> | ||
| <StringifiedJobCondition condition={condition} truncate /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility of the hover trigger
The hover trigger div should have appropriate ARIA attributes and keyboard interaction support.
- <div className="cursor-pointer rounded-lg bg-inherit text-xs text-muted-foreground">
+ <div
+ role="button"
+ tabIndex={0}
+ aria-label="View full condition"
+ onKeyDown={(e) => e.key === 'Enter' && e.currentTarget.click()}
+ className="cursor-pointer rounded-lg bg-inherit text-xs text-muted-foreground"
+ >📝 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="cursor-pointer rounded-lg bg-inherit text-xs text-muted-foreground"> | |
| <StringifiedJobCondition condition={condition} truncate /> | |
| </div> | |
| <div | |
| role="button" | |
| tabIndex={0} | |
| aria-label="View full condition" | |
| onKeyDown={(e) => e.key === 'Enter' && e.currentTarget.click()} | |
| className="cursor-pointer rounded-lg bg-inherit text-xs text-muted-foreground" | |
| > | |
| <StringifiedJobCondition condition={condition} truncate /> | |
| </div> |
| return null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve handling of unknown condition types
Instead of silently returning null for unknown condition types, consider displaying an error badge to make debugging easier.
- return null;
+ return (
+ <ConditionBadge>
+ <span className="text-red-500">Unknown condition type</span>
+ </ConditionBadge>
+ );Committable suggestion was skipped due to low confidence.
| const total = await ctx.db | ||
| .select({ | ||
| count: countDistinct(releaseJobTrigger.id), | ||
| }) | ||
| .from(releaseJobTrigger) | ||
| .innerJoin(job, eq(releaseJobTrigger.jobId, job.id)) | ||
| .innerJoin(target, eq(releaseJobTrigger.targetId, target.id)) | ||
| .innerJoin(release, eq(releaseJobTrigger.releaseId, release.id)) | ||
| .innerJoin(deployment, eq(release.deploymentId, deployment.id)) | ||
| .innerJoin( | ||
| environment, | ||
| eq(releaseJobTrigger.environmentId, environment.id), | ||
| ) | ||
| .innerJoin(system, eq(environment.systemId, system.id)) | ||
| .where( | ||
| and( | ||
| eq(system.workspaceId, input.workspaceId), | ||
| jobMatchesCondition(ctx.db, input.filter), | ||
| ), | ||
| ) | ||
| .then(takeFirst) | ||
| .then((t) => t.count); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the total count query.
The total count query duplicates all joins from the main query, which could impact performance with large datasets. Consider using a subquery or materialized view for better efficiency.
- const total = await ctx.db
- .select({
- count: countDistinct(releaseJobTrigger.id),
- })
- .from(releaseJobTrigger)
- .innerJoin(job, eq(releaseJobTrigger.jobId, job.id))
- .innerJoin(target, eq(releaseJobTrigger.targetId, target.id))
- .innerJoin(release, eq(releaseJobTrigger.releaseId, release.id))
- .innerJoin(deployment, eq(release.deploymentId, deployment.id))
- .innerJoin(
- environment,
- eq(releaseJobTrigger.environmentId, environment.id),
- )
- .innerJoin(system, eq(environment.systemId, system.id))
- .where(
- and(
- eq(system.workspaceId, input.workspaceId),
- jobMatchesCondition(ctx.db, input.filter),
- ),
- )
- .then(takeFirst)
- .then((t) => t.count);
+ const total = await ctx.db
+ .select({
+ count: countDistinct(releaseJobTrigger.id),
+ })
+ .from(
+ ctx.db
+ .select({
+ id: releaseJobTrigger.id,
+ })
+ .from(releaseJobTrigger)
+ .innerJoin(job, eq(releaseJobTrigger.jobId, job.id))
+ .innerJoin(release, eq(releaseJobTrigger.releaseId, release.id))
+ .innerJoin(deployment, eq(release.deploymentId, deployment.id))
+ .innerJoin(system, eq(deployment.systemId, system.id))
+ .where(
+ and(
+ eq(system.workspaceId, input.workspaceId),
+ jobMatchesCondition(ctx.db, input.filter),
+ ),
+ )
+ .as("filtered_triggers"),
+ )
+ .then(takeFirst)
+ .then((t) => t.count);Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
Release Notes
New Features
JobTablecomponent for displaying job triggers within a workspace.JobConditionDialogfor managing job conditions in a user-friendly dialog interface.JobTargetConditionRenderfor selecting job target conditions.ReleaseVersionConditionRendercomponent for managing version conditions.VersionConditionRendercomponent for rendering version conditions.Bug Fixes
Documentation
Refactor