add version selector rule #458
Conversation
WalkthroughThis pull request integrates deployment version selector functionality into the system. It adds a new rule in the rule engine, updates the API endpoints to manage version selectors, and revises database schema and relations. The UI is enhanced with new components and sidebar entries for version channels, while tests covering the new logic are introduced. Additionally, a minor update in job dispatch flow marks valid jobs as in progress once their variables are resolved. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EvaluateRepo as EvaluateRepository
participant Selector as versionSelector
participant Rule as DeploymentVersionSelectorRule
participant DB as Database
Client->>EvaluateRepo: Request policy evaluation
EvaluateRepo->>Selector: Invoke versionSelector(policy)
Selector->>Rule: Create DeploymentVersionSelectorRule(getApplicableVersionIds)
Rule->>Rule: Execute filter(context, releases)
Rule->>DB: Query applicable version IDs
DB-->>Rule: Return matching IDs
Rule-->>Selector: Return filtered results
Selector-->>EvaluateRepo: Append rule outcome
EvaluateRepo-->>Client: Return evaluation result
sequenceDiagram
participant User
participant Dialog as VersionChannelRuleDialog
participant API as PolicyRouter/API
participant DB as Database
User->>Dialog: Open creation/edit dialog
Dialog->>API: Fetch eligible policies / Submit form data
API->>DB: Create/Update deployment version selector
DB-->>API: Confirm mutation
API-->>Dialog: Return success response
Dialog-->>User: Display success notification
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 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: 0
🧹 Nitpick comments (2)
packages/rule-engine/src/evaluate.ts (1)
99-99: Consider ordering rules carefully.Combining
denyWindowsrules before or after the version selector may affect which releases are excluded first. If the evaluation order matters, consider reordering the spread arrays to reflect the intended logic.packages/rule-engine/src/rules/deployment-version-selector-rule.ts (1)
49-66: Potential performance concerns with large version sets.
inArraywith many IDs could become costly. If dealing with large version lists, consider chunking or indexing to optimize DB queries.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rule-engine/src/evaluate.ts(3 hunks)packages/rule-engine/src/rules/deployment-version-selector-rule.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/rule-engine/src/evaluate.tspackages/rule-engine/src/rules/deployment-version-selector-rule.ts
🧬 Code Definitions (1)
packages/rule-engine/src/evaluate.ts (2)
packages/rule-engine/src/types.ts (1)
Policy(66-69)packages/rule-engine/src/rules/deployment-version-selector-rule.ts (2)
DeploymentVersionSelectorRule(20-47)getApplicableVersionIds(49-66)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (7)
packages/rule-engine/src/evaluate.ts (2)
9-12: Looks good on the new imports.Importing
DeploymentVersionSelectorRuleandgetApplicableVersionIdshere cleanly integrates the version selector logic. No issues noted.
26-34: Function name clarity.The
versionSelectorfunction is straightforward, returning a new rule instance ifpolicyis not null. The approach is concise and easy to maintain.packages/rule-engine/src/rules/deployment-version-selector-rule.ts (5)
1-4: Database imports usage is acceptable.Imports from
@ctrlplane/dband@ctrlplane/db/clientappear safe. No injection concerns noted given the parameterized usage in queries.
5-11: Relevant type imports established.The imported types ensure clarity in function signatures and rule definitions.
13-13: Rejection reason defined.A concise, meaningful constant for rejection. Future customization/localization might be desirable, but this is fine for now.
15-18: Type signature is flexible.Allowing both
Promise<string[]>andstring[]return types provides versatility for synchronous or asynchronous retrieval. Make sure all consumers handle both cases.
20-47: Rule implementation is solid.Implements the
DeploymentResourceRuleinterface cleanly. The filtering logic is straightforward, maintaining rejection reasons in a map. Overall, well-structured.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/page.tsx (1)
23-37: Version selectors page implementationThe VersionSelectorsPage component correctly fetches workspace data and policies, with proper error handling for the not found case. The filtering of policies to include only those with a deploymentVersionSelector is appropriate.
Consider removing the eslint-disable comment on line 35 by using a more type-safe approach:
- // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - (p) => p.deploymentVersionSelector, + (p) => p.deploymentVersionSelector !== null && p.deploymentVersionSelector !== undefined,apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/_components/VersionSelectorTable.tsx (2)
29-43: Consider using specific types instead of anyThe
deploymentVersionSelectorproperty is typed asany, which reduces type safety. Consider defining a more specific type structure based on the actual data shape.type DeploymentVersionSelector = { id: string; name: string; description: string | null; - deploymentVersionSelector: any; + deploymentVersionSelector: { + type: string; + operator?: string; + conditions?: Array<unknown>; + value?: string; + key?: string; + }; };
49-91: Improve readability of condition summary renderingThe
renderConditionSummaryfunction uses nested ternary operators in multiple places, which can make the code harder to read and maintain. Consider refactoring to use more explicit if statements or switch cases.function renderConditionSummary(condition: any): string { if (!condition) return "No conditions"; // If it's a comparison condition (and/or) if (condition.type === "comparison") { const operator = condition.operator === "and" ? "AND" : "OR"; if (condition.conditions.length === 0) return "Empty condition"; return `${operator} (${condition.conditions.length} condition${condition.conditions.length !== 1 ? "s" : ""})`; } // For tag conditions if (condition.type === "tag") { const op = condition.operator === "equals" ? "=" : "~"; return `tag ${op} ${condition.value}`; } // For metadata conditions if (condition.type === "metadata") { const op = condition.operator === "equals" ? "=" : "~"; return `metadata.${condition.key} ${op} ${condition.value}`; } // For version conditions if (condition.type === "version") { const op = condition.operator === "equals" ? "=" : "~"; return `version ${op} ${condition.value}`; } // For created date conditions if (condition.type === "created-at") { - const op = - condition.operator === "before" - ? "<" - : condition.operator === "after" - ? ">" - : condition.operator === "before-or-on" - ? "<=" - : ">="; + let op; + switch (condition.operator) { + case "before": + op = "<"; + break; + case "after": + op = ">"; + break; + case "before-or-on": + op = "<="; + break; + default: // assuming "after-or-on" + op = ">="; + break; + } return `created ${op} ${new Date(condition.value).toLocaleDateString()}`; } return "Complex condition"; }packages/rule-engine/src/rules/__tests__/deployment-version-selector-rule.test.ts (2)
122-131: Consider adding more test cases for getApplicableVersionIds.While you've tested the null selector case, consider adding tests for:
- Empty version IDs array
- A selector that returns an empty array
- Error handling scenarios where the function throws an exception
// Example additional test cases: it("should handle empty version IDs array", () => { const func = getApplicableVersionIds(null); const result = func(context, []); expect(result).toEqual([]); }); it("should handle selector returning empty array", () => { const mockSelector = { // Configuration for a selector that returns no versions }; const func = getApplicableVersionIds(mockSelector); const result = func(context, ["ver-1", "ver-2"]); expect(result).toEqual([]); }); it("should handle errors in selector processing", async () => { const errorSelector = { // Configuration that would cause an error }; const func = getApplicableVersionIds(errorSelector); // Test depending on your error handling approach await expect(func(context, ["ver-1"])).rejects.toThrow(); // Or if you handle errors by returning all versions: // expect(await func(context, ["ver-1"])).toEqual(["ver-1"]); });
1-132: Consider adding an integration test with a real selector configuration.The current tests focus on mocking
getApplicableVersionIdsbehavior, but an integration test with a realistic selector configuration would verify that the actual logic for parsing and applying selectors works correctly.it("should apply a real version selector configuration", async () => { // Create a real selector configuration const versionSelector = { versions: ["v1.0.0"], // or whatever format your selector uses }; // Use the actual getApplicableVersionIds function const rule = new DeploymentVersionSelectorRule( getApplicableVersionIds(versionSelector) ); const result = await rule.filter(context, releases); // Verify expected behavior expect(result.allowedReleases.length).toBe(1); expect(result.allowedReleases.at(0)?.id).toBe("rel-1"); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/PolicyTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/rule-themes.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/layout.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/page.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/_components/VersionSelectorTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/page.tsx(1 hunks)apps/webservice/src/app/urls.ts(1 hunks)packages/api/src/router/policy.ts(2 hunks)packages/rule-engine/src/rules/__tests__/deployment-version-selector-rule.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/urls.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/PolicyTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/rule-themes.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/page.tsxpackages/api/src/router/policy.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/_components/VersionSelectorTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/layout.tsxpackages/rule-engine/src/rules/__tests__/deployment-version-selector-rule.test.ts
🧬 Code Definitions (3)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/page.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/_components/VersionSelectorTable.tsx (1)
VersionSelectorTable(93-192)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/_components/VersionSelectorTable.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/rule-themes.tsx (1)
getTypeColorClass(61-82)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/layout.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/SidebarLink.tsx (1)
SidebarLink(9-32)apps/webservice/src/app/urls.ts (1)
urls(214-214)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (23)
apps/webservice/src/app/urls.ts (1)
48-48: Good addition for enhancing URL construction flexibility.The
pathmethod provides a clean way to construct policy URLs with any subpath, which is useful for the new version selector feature.apps/webservice/src/app/[workspaceSlug]/(app)/policies/layout.tsx (2)
11-11: LGTM: New icon import for version selectors.The
IconTagimport is appropriately added for the new version selectors feature.
79-92: Well-structured sidebar addition for version selectors.The new sidebar group for "Deployment Rules" with the "Version Selectors" link follows the existing pattern and correctly uses the newly added
pathmethod from the URL utilities.apps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/PolicyTable.tsx (1)
38-39: LGTM: Added version selector rule detection.The condition to check for
policy.deploymentVersionSelectorfollows the same pattern as the existing deny window check, and the ESLint disable comment is appropriate since TypeScript might be inferring the property is always defined.apps/webservice/src/app/[workspaceSlug]/(app)/policies/page.tsx (3)
12-12: LGTM: Added icon import for version selectors.The
IconTagimport is correctly added to support the new version selectors UI element.
54-56: LGTM: Added count for version selectors.The count calculation for version selectors correctly filters policies with the
deploymentVersionSelectorproperty.
75-81: Well-structured addition to rule categories.The "Version Selectors" category is properly integrated into the existing
ruleCategoriesarray with appropriate title, icon, count reference, and description.packages/api/src/router/policy.ts (2)
32-36: Policy queries now include deploymentVersionSelectorThe addition of
deploymentVersionSelectorto thewithclause in thelistquery is a good enhancement that supports the version selector functionality. This change ensures that when policies are fetched, their associated version selector data is included in the response.
49-53: deploymentVersionSelector correctly added to byId querySimilar to the list query, the byId query now includes the deploymentVersionSelector in its
withclause. This change maintains consistency between the two query methods and ensures that version selector data is available when fetching a specific policy.apps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/rule-themes.tsx (5)
9-9: Added IconTag import for the version selector iconThe IconTag is appropriately imported to represent the version selector visually.
12-12: RuleType type updated to include version-selectorThe RuleType has been properly extended to include "version-selector" as a specific string literal type, which helps with type safety throughout the application.
30-31: Added icon representation for version-selectorThe version-selector icon implementation follows the same pattern as other rule types, using the IconTag with appropriate styling.
53-54: Added label for version-selectorThe human-readable label "Version Selector" has been added for the version-selector rule type, maintaining consistency with the labeling approach used for other rule types.
77-78: Added styling for version-selectorThe color class implementation for version-selector follows the established pattern with a distinct indigo color scheme, providing visual differentiation from other rule types.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/page.tsx (2)
75-95: Well-structured explanatory cardThe explanatory card provides clear information about version selectors, which is helpful for users. The use of IconTag maintains consistent visual language with the rule-themes definitions.
97-97: Passing filtered policies to table componentThe VersionSelectorTable component is correctly used with the filtered policies passed as props.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/_components/VersionSelectorTable.tsx (2)
156-160: Disabled switch componentThe Switch component for policy status is disabled. If this is intentional (for a read-only view), consider adding a tooltip to explain why the status cannot be changed, or implement the toggle functionality if it's meant to be interactive.
Is the disabled state of the Switch component intentional? If enabling/disabling policies is supported elsewhere in the application, this should be consistent.
138-153: Good use of Tooltip for condition detailsThe tooltip showing the full JSON structure of the condition provides good developer/power-user experience while keeping the UI clean with a summarized version visible by default.
packages/rule-engine/src/rules/__tests__/deployment-version-selector-rule.test.ts (5)
1-12: Imports look well-structured.The imports are properly organized, separating test utilities, types, and the components under test.
14-63: Test fixtures are comprehensive and well-structured.The test setup provides a good foundation with realistic test data. The sample releases have different version IDs, tags, and metadata, which will help test various filtering scenarios.
65-82: Good test case for the default behavior.This test properly verifies that all releases are allowed when no version selector is provided, which is an important edge case to cover.
84-100: Well-structured test for core filtering functionality.This test effectively verifies that releases are properly filtered based on the version selector, including checking rejection reasons.
102-120: Good coverage of async behavior.Testing the asynchronous nature of the
getApplicableVersionIdsfunction ensures that the rule will work correctly in real-world scenarios where database queries might be involved.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-selectors/page.tsx
Outdated
Show resolved
Hide resolved
...rc/app/[workspaceSlug]/(app)/policies/version-selectors/_components/VersionSelectorTable.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsx (1)
52-114: Consider adding a loading state while data is being fetched.Since this component fetches data asynchronously, it's good practice to show a loading state while waiting for the data. Next.js supports React Suspense boundaries which can be used to show a loading UI.
// In a parent layout file or in this component +import { Suspense } from "react"; +import { Skeleton } from "@ctrlplane/ui/skeleton"; // Wrap the component or relevant parts with Suspense +<Suspense fallback={<VersionChannelsPageSkeleton />}> <VersionChannelsPage params={params} /> +</Suspense> // Create a skeleton component +function VersionChannelsPageSkeleton() { + return ( + <div className="flex h-full flex-col"> + <div className="flex items-center gap-2 p-4 border-b"> + <Skeleton className="h-4 w-40" /> + </div> + <div className="p-6"> + <Skeleton className="h-6 w-48 mb-2" /> + <Skeleton className="h-4 w-96 mb-6" /> + <Skeleton className="h-32 w-full mb-8" /> + <Skeleton className="h-64 w-full" /> + </div> + </div> + ); +}apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/_components/VersionChannelTable.tsx (1)
117-118: Clarify the nesteddeploymentVersionSelectorproperty naming.The property access
policy.deploymentVersionSelector.deploymentVersionSelectorappears redundant and potentially confusing. Consider renaming one of these for clarity or adding a comment explaining this structure:- const condition = - policy.deploymentVersionSelector.deploymentVersionSelector; + // The outer deploymentVersionSelector is the object, + // inner one is the actual rule condition + const condition = policy.deploymentVersionSelector.deploymentVersionSelector; // Or alternatively, rename for clarity: + const versionSelectorRule = policy.deploymentVersionSelector.deploymentVersionSelector;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/layout.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/page.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/_components/VersionChannelTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/layout.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/_components/VersionChannelTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsx
🧬 Code Definitions (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/_components/VersionChannelTable.tsx (1)
VersionChannelTable(87-188)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (6)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsx (2)
76-79: Add functionality to the "Create Version Channel Rule" button.The "Create Version Channel Rule" button is currently static with no click handler or navigation link. Consider implementing the intended functionality:
- <Button variant="outline" size="sm"> + <Button + variant="outline" + size="sm" + asChild + > + <Link href={`/${workspaceSlug}/policies/create?type=version-channel`}> <IconPlus className="mr-2 h-4 w-4" /> Create Version Channel Rule + </Link> </Button>Or if it should trigger a modal/dialog:
- <Button variant="outline" size="sm"> + <Button + variant="outline" + size="sm" + onClick={() => { + // Open modal or navigate to creation page + }} + > <IconPlus className="mr-2 h-4 w-4" /> Create Version Channel Rule </Button>
112-112: Good use of the filtered policies to display version channels.The component correctly filters the policies to only show those with version channels and passes them to the
VersionChannelTablecomponent. This ensures users only see relevant information.apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/_components/VersionChannelTable.tsx (4)
174-178: Implement removal of channel rule or disable the menu item.The "Remove Channel Rule" dropdown item has a TODO comment and doesn't have any functionality. Either:
- Implement the removal functionality:
- <DropdownMenuItem className="text-destructive focus:text-destructive"> - <Trash2 className="mr-2 h-4 w-4" /> - Remove Channel Rule - {/* TODO: Implement removal of just the channel rule */} + <DropdownMenuItem + className="text-destructive focus:text-destructive" + onClick={() => handleRemoveChannelRule(policy.id)} + > + <Trash2 className="mr-2 h-4 w-4" /> + Remove Channel Rule </DropdownMenuItem>
- Or disable the menu item until implementation is complete:
- <DropdownMenuItem className="text-destructive focus:text-destructive"> + <DropdownMenuItem + className="text-destructive focus:text-destructive" + disabled + title="Coming soon" + > <Trash2 className="mr-2 h-4 w-4" /> Remove Channel Rule {/* TODO: Implement removal of just the channel rule */} </DropdownMenuItem>
94-101: Great empty state handling with appropriate UI feedback.The component correctly handles the case when no policies with version channels are found, providing clear feedback to the user with a well-designed empty state.
131-146: Well-implemented tooltip for displaying complex condition details.The use of tooltips to show the full JSON representation of the condition while displaying a more user-friendly summary in the table is an excellent UX decision. It allows users to see detailed information without cluttering the UI.
43-85: Robust condition summary function supporting multiple condition types.The
renderConditionSummaryfunction handles a variety of condition types with appropriate formatting. The function is well-structured with clear logic for each condition type, making it maintainable and extensible.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsx
Outdated
Show resolved
Hide resolved
.../src/app/[workspaceSlug]/(app)/policies/version-channels/_components/VersionChannelTable.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rule-engine/src/evaluate.ts (1)
32-150: Consider documenting the new versionSelector function and rule evaluation order.The new versionSelector function lacks documentation comments, unlike some other functions in the file. Also, the order of rules in the evaluation pipeline may be significant, but there's no documentation explaining the reasoning for this specific sequence.
Consider adding JSDoc comments to the new function and explaining the rule evaluation order:
+/** + * Creates deployment version selector rules based on the provided policy. + * + * @param policy - The policy containing deployment version selector configuration + * @returns An array of DeploymentVersionSelectorRule instances, or an empty array if the policy is null + */ const versionSelector = (policy: Policy | null) => policy == null ? [] : [ new DeploymentVersionSelectorRule( getApplicableVersionIds(policy.deploymentVersionSelector), ), ]; // Later in evaluateRepository + // Rules are evaluated in order: deny windows first, then version selectors, + // followed by various approval rules const rules = [ ...denyWindows(policy), ...versionSelector(policy), ...versionUserApprovalRule(policy?.versionUserApprovals), ...versionAnyApprovalRule(policy?.versionAnyApprovals), ...versionRoleApprovalRule(policy?.versionRoleApprovals), ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/event-worker/src/workers/job-dispatch/github.ts(1 hunks)packages/job-dispatch/src/job-dispatch.ts(1 hunks)packages/rule-engine/src/evaluate.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/job-dispatch/src/job-dispatch.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/rule-engine/src/evaluate.tsapps/event-worker/src/workers/job-dispatch/github.ts
🧬 Code Definitions (2)
packages/rule-engine/src/evaluate.ts (2)
packages/rule-engine/src/types.ts (1)
Policy(66-72)packages/rule-engine/src/rules/deployment-version-selector-rule.ts (2)
DeploymentVersionSelectorRule(20-47)getApplicableVersionIds(49-66)
apps/event-worker/src/workers/job-dispatch/github.ts (2)
apps/webservice/src/app/api/github/octokit.ts (1)
octokit(12-22)packages/db/src/schema/job.ts (1)
JobStatus(136-136)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
apps/event-worker/src/workers/job-dispatch/github.ts (1)
177-190: Good improvement to job status tracking.The change from a direct await to a Promise chain adds the crucial step of updating the job status to
InProgressupon successful workflow dispatch. This improves the job lifecycle tracking without sacrificing error handling.The implementation:
- Maintains clear and predictable control flow using Promise chains
- Preserves error handling via the surrounding try/catch block
- Properly updates job state at the appropriate time in the workflow
This change aligns with the provided coding guidelines which explicitly allow Promise chains as an acceptable pattern.
packages/rule-engine/src/evaluate.ts (3)
9-12: Import declaration follows established patterns.The new imports for
DeploymentVersionSelectorRuleandgetApplicableVersionIdsare structured correctly and follow the existing import pattern in the file.
32-39: LGTM: Well-implemented version selector function.The new
versionSelectorfunction follows the same pattern as other rule generator functions in the file:
- Takes a nullable policy parameter
- Returns an empty array for null policies
- Creates the appropriate rule instance when a policy is provided
- Properly handles the policy.deploymentVersionSelector property
The implementation is concise and aligns well with the codebase style.
146-146: Correctly integrates version selector rules.The new version selector rules are properly integrated into the rule engine evaluation pipeline, placed appropriately in the sequence of rules to be evaluated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/api/src/router/policy.ts (3)
230-246: Consider unique constraint collisions.
These lines introduce thecreateDeploymentVersionSelectorendpoint. BecausepolicyIdis unique inpolicy_rule_deployment_version_selector, multiple inserts for the same policy will likely fail with a database constraint error. Consider handling this gracefully (e.g., returning a user-facing message) if attempting to create more than one version selector per policy.
248-271: Validate existence of the record when updating.
Currently, if no deployment version selector record exists for the specifiedpolicyId, the DB update will return an empty array, causing.then(takeFirst)to throw. Consider handling this case gracefully and returning a clear error or fallback response if the record doesn't exist.
273-288: Handle a missing record scenario when deleting.
If a version selector does not exist for the specifiedpolicyId, thedeletequery could return an empty result, which again triggers.then(takeFirst)to throw. You might want to return a 404 or a user-friendly message in such a case.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/api/src/router/policy.ts(4 hunks)packages/db/src/schema/policy.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/policy.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/policy.ts
🧬 Code Definitions (1)
packages/api/src/router/policy.ts (3)
packages/api/src/trpc.ts (1)
protectedProcedure(150-150)packages/db/src/schema/policy.ts (3)
createPolicyRuleDeploymentVersionSelector(114-115)policyRuleDeploymentVersionSelector(55-74)updatePolicyRuleDeploymentVersionSelector(120-121)packages/db/src/common.ts (1)
takeFirst(9-13)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (5)
packages/api/src/router/policy.ts (5)
8-8: Import usage looks consistent.
ThecreatePolicyRuleDeploymentVersionSelectorimport is properly referenced later in the code. No immediate issues found.
12-12: Import usage looks consistent.
ThepolicyRuleDeploymentVersionSelectorimport is used as expected. No issues found.
16-16: Import usage looks consistent.
TheupdatePolicyRuleDeploymentVersionSelectorimport is referenced correctly in the update method.
35-39: No concerns with includingdeploymentVersionSelectorin the policy list query.
The addition ofdeploymentVersionSelector: trueis consistent and appears to align with retrieving relevant policy data in one query.
52-56: Ensures consistency with the policy retrieval by ID.
IncludingdeploymentVersionSelectorin thewithclause mirrors the pattern used in the list query. This keeps retrieval consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/RuleDialog.tsx (4)
92-93: Remove or implement the TODO comment.There's a TODO comment about adding a createMutation if needed. Since this component is for editing an existing rule, it's unlikely that you need a createMutation here. Consider removing this comment if it's no longer relevant.
- // TODO: Add createMutation if needed
99-101: Clarify the comment about create vs. update logic.The comment suggests uncertainty about whether to create or update based on the initial state. Since this is specifically an edit dialog for an existing rule (as indicated by the component name and the fact that it receives an existing policy), the logic should always be update. Consider removing this comment to avoid confusion.
- // Decide whether to create or update based on initial state? - // For now, assuming update as it's triggered from an existing rule updateMutation.mutate({
198-198: Remove unnecessary Fragment.The Fragment element is redundant since it contains no children. You can remove it and use
nullor an empty string instead.- <></> + {null}🧰 Tools
🪛 Biome (1.9.4)
[error] 198-198: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
82-83: Consider uncommenting the invalidation call or remove if unnecessary.There's a commented-out invalidation call for the specific policy. If this invalidation is necessary to ensure UI consistency after updates, consider uncommenting it. Otherwise, if it's truly unnecessary, the comment should be removed to keep the codebase clean.
- // Consider invalidating policy.byId if used elsewhere - // utils.policy.byId.invalidate({ id: policy.id }); + utils.policy.byId.invalidate(policy.id);apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/RuleTable.tsx (2)
63-64: Consider more specific invalidation.The comment suggests being more specific with invalidation queries by using workspaceId if the list was fetched with it. This would be more efficient than invalidating all policy lists. Consider implementing this optimization if possible.
- // Consider invalidating by workspaceId if list was fetched with it - queryClient.invalidateQueries({ queryKey: listPoliciesQueryKey }); + // Assuming policies are fetched with workspaceId + if (policies.length > 0) { + const workspaceId = policies[0].workspaceId; + queryClient.invalidateQueries({ + queryKey: [...listPoliciesQueryKey, workspaceId] + }); + } else { + queryClient.invalidateQueries({ queryKey: listPoliciesQueryKey }); + }
80-88: Consider removing redundant empty state handling.The comment suggests that this empty state handling is already managed by the parent page component. If that's the case, consider removing this check to reduce code duplication and maintain a single source of truth for empty state handling.
- if (policies.length === 0) { - // This case should technically be handled by the parent page now, - // but include a fallback just in case. - return ( - <p className="p-6 text-center text-sm text-muted-foreground"> - No policies with Version Selector rules found. - </p> - ); - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/CreateRuleDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/RuleDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/RuleTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/PolicyTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/page.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/_components/PolicyTable.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/policies/page.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/RuleTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/RuleDialog.tsxapps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/CreateRuleDialog.tsx
🪛 Biome (1.9.4)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/RuleDialog.tsx
[error] 198-198: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/version-channels/page.tsx (1)
1-120: LGTM! Well-structured component with proper error handling.The component is well organized with clear separation of concerns between the main page and the data rendering component. It handles loading states appropriately with Skeleton components and provides meaningful feedback for edge cases (no policies found).
While there's no explicit try/catch blocks for API calls, the component does handle loading and error states through conditional rendering, which aligns with the coding guidelines that allow for alternative error handling patterns beyond try/catch blocks.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/policy/version-selector/CreateRuleDialog.tsx (1)
1-255: LGTM! Well-implemented form dialog with proper validation and error handling.The component follows best practices for React form implementation:
- Uses react-hook-form with Zod for validation
- Handles loading and error states appropriately
- Provides clear user feedback through toast notifications
- Properly resets form state when the dialog is closed
The form submission logic correctly validates that a valid deployment version condition is set before proceeding with the mutation, and the UI is clear and intuitive.
Summary by CodeRabbit