chore: opa plan validation ui#1097
Open
adityachoudhari26 wants to merge 1 commit intomainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a validation-focused UI layer to plan results. The TRPC backend now aggregates validation statistics per plan result and provides a dedicated endpoint for detailed validations. The frontend adds a second tab to the plan diff dialog for viewing validation results with pass/fail badges, and a validations cell in the results table that displays counts and enables navigation to the new validations tab. Changes
Sequence DiagramsequenceDiagram
participant User
participant Table as Plan Results Table
participant Dialog as PlanDiffDialog
participant TRPCClient as TRPC Client
participant Backend as Backend API
User->>Table: Click validations cell
Table->>TRPCClient: onOpenResult(resultId, "validations")
TRPCClient->>Backend: resultValidations({ deploymentId, resultId })
Backend->>Backend: Fetch validation entries & rule metadata
Backend-->>TRPCClient: Validation details + descriptions
TRPCClient->>Dialog: Open with initialTab="validations"
Dialog->>Dialog: Render ValidationsTab with badges & messages
Dialog-->>User: Display validation results
User->>Table: Click row (hasChanges or validations.total > 0)
Table->>TRPCClient: onOpenResult(resultId, "diff")
TRPCClient->>Backend: results({ deploymentId })
Backend-->>TRPCClient: Plan diff data
TRPCClient->>Dialog: Open with initialTab="diff"
Dialog->>Dialog: Render DiffTab with Monaco YAML diff
Dialog-->>User: Display diff with Split/Unified toggle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds plan validation visibility (OPA plan validation) to the deployment plan results UI by returning validation counts with plan results and exposing a new resultValidations endpoint to fetch per-result validation details for display in the diff dialog.
Changes:
- Add aggregated validation counts to
deployment.plans.resultsresponses. - Add a new
deployment.plans.resultValidationsTRPC query for per-result validation details (rule name/description, pass/fail, violations). - Update the plan results UI to show a “Validations” column and add a “Validations” tab to the plan diff dialog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/trpc/src/routes/deployment-plans.ts | Adds validation counts to plan results and a new resultValidations query. |
| apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx | Displays validation counts in the results table and wires opening the dialog to specific tabs. |
| apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx | Adds a “Validations” tab that fetches and renders rule evaluation results alongside the diff. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+135
to
+142
| const hasValidations = result.validations.total > 0; | ||
| const isClickable = result.hasChanges === true || hasValidations; | ||
| return ( | ||
| <TableRow | ||
| className={cn("hover:bg-muted/50", isClickable && "cursor-pointer")} | ||
| onClick={isClickable ? () => onViewDiff(result.resultId) : undefined} | ||
| onClick={ | ||
| isClickable ? () => onOpenResult(result.resultId, "diff") : undefined | ||
| } |
Comment on lines
138
to
+142
| <TableRow | ||
| className={cn("hover:bg-muted/50", isClickable && "cursor-pointer")} | ||
| onClick={isClickable ? () => onViewDiff(result.resultId) : undefined} | ||
| onClick={ | ||
| isClickable ? () => onOpenResult(result.resultId, "diff") : undefined | ||
| } |
Comment on lines
+89
to
+90
| {validation.violations.map((violation, i) => ( | ||
| <li key={i} className="font-mono text-red-600 dark:text-red-400"> |
Comment on lines
+293
to
+308
| const owner = await ctx.db | ||
| .select({ deploymentId: schema.deploymentPlan.deploymentId }) | ||
| .from(schema.deploymentPlanTargetResult) | ||
| .innerJoin( | ||
| schema.deploymentPlanTarget, | ||
| eq( | ||
| schema.deploymentPlanTargetResult.targetId, | ||
| schema.deploymentPlanTarget.id, | ||
| ), | ||
| ) | ||
| .innerJoin( | ||
| schema.deploymentPlan, | ||
| eq(schema.deploymentPlanTarget.planId, schema.deploymentPlan.id), | ||
| ) | ||
| .where(eq(schema.deploymentPlanTargetResult.id, input.resultId)) | ||
| .then(takeFirstOrNull); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx (1)
81-87: Convert function components to typed React.FC declarations.The file contains 6 function declarations that are React components. Per codebase conventions, these should use
React.FCwith explicit typing:const ComponentName: React.FC<Props> = ({ prop }) => {}.Affected components:
- DiffStats (line 45)
- ChangesCell (line 68)
- ValidationsCell (line 81)
- ResultsTableHeader (line 113)
- ResultsTableRow (line 128)
- NoResults (line 163)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/deployments/page`.$deploymentId.plans.$planId.tsx around lines 81 - 87, Convert the listed function components to typed React.FC declarations: replace each function declaration (DiffStats, ChangesCell, ValidationsCell, ResultsTableHeader, ResultsTableRow, NoResults) with a const arrow component using explicit props typing, e.g. const ComponentName: React.FC<Props> = ({ ... }) => { ... }; ensure you reuse the same prop types (e.g. the existing Result["validations"] type for ValidationsCell) and preserve prop names and implementation body, export behavior and any default props or handlers (like onClick) unchanged.apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx (1)
18-25: Consider adoptingReact.FCtyping andinterfacefor props to align with documented conventions.While the code works correctly, the repository style guide documents preferences for
React.FC-typed exported components andinterfacefor public props. However, note that the codebase currently usesexport functiondeclarations as the dominant pattern across similar components, suggesting these guidelines are aspirational rather than uniformly enforced. Adopt these patterns if the team intends to standardize on them project-wide, or deprioritize if functional component declarations remain the preferred convention.Affected segments:
PlanDiffDialogProps(line 18),ValidationsTab(line 33),ValidationItem(line 73),DiffTab(line 100),PlanDiffDialog(line 156).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx` around lines 18 - 25, Change the exported components to use React.FC and convert the public props type aliases into interfaces: replace the type alias PlanDiffDialogProps with interface PlanDiffDialogProps and update ValidationsTab, ValidationItem, DiffTab and PlanDiffDialog function declarations to use React.FC<...> (e.g., const PlanDiffDialog: React.FC<PlanDiffDialogProps> or export const PlanDiffDialog: React.FC<PlanDiffDialogProps>) so props are typed via the new interfaces; keep the same prop names and signatures (resultId, deploymentId, title, open, initialTab, onOpenChange) and ensure exported names (ValidationsTab, ValidationItem, DiffTab, PlanDiffDialog) still match their existing exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/app/routes/ws/deployments/page`.$deploymentId.plans.$planId.tsx:
- Around line 138-142: The TableRow currently uses onClick for navigation which
is pointer-only; make rows keyboard-accessible by, when isClickable is true,
adding focusability (e.g., tabIndex={0}), a proper semantic role (e.g.,
role="button" or adjust per semantics) and an onKeyDown handler that listens for
Enter and Space and calls onOpenResult(result.resultId, "diff") (for Space
ensure preventDefault to avoid scrolling). Update the TableRow props where
isClickable is used so it conditionally adds tabIndex, role, onKeyDown, and
keeps the existing onClick and cursor/focus styles so keyboard users can both
focus and activate the row.
---
Nitpick comments:
In `@apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx`:
- Around line 18-25: Change the exported components to use React.FC and convert
the public props type aliases into interfaces: replace the type alias
PlanDiffDialogProps with interface PlanDiffDialogProps and update
ValidationsTab, ValidationItem, DiffTab and PlanDiffDialog function declarations
to use React.FC<...> (e.g., const PlanDiffDialog: React.FC<PlanDiffDialogProps>
or export const PlanDiffDialog: React.FC<PlanDiffDialogProps>) so props are
typed via the new interfaces; keep the same prop names and signatures (resultId,
deploymentId, title, open, initialTab, onOpenChange) and ensure exported names
(ValidationsTab, ValidationItem, DiffTab, PlanDiffDialog) still match their
existing exports.
In `@apps/web/app/routes/ws/deployments/page`.$deploymentId.plans.$planId.tsx:
- Around line 81-87: Convert the listed function components to typed React.FC
declarations: replace each function declaration (DiffStats, ChangesCell,
ValidationsCell, ResultsTableHeader, ResultsTableRow, NoResults) with a const
arrow component using explicit props typing, e.g. const ComponentName:
React.FC<Props> = ({ ... }) => { ... }; ensure you reuse the same prop types
(e.g. the existing Result["validations"] type for ValidationsCell) and preserve
prop names and implementation body, export behavior and any default props or
handlers (like onClick) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1d7acca-3acb-46cc-b26a-7669a6f271f8
📒 Files selected for processing (3)
apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsxapps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsxpackages/trpc/src/routes/deployment-plans.ts
Comment on lines
138
to
+142
| <TableRow | ||
| className={cn("hover:bg-muted/50", isClickable && "cursor-pointer")} | ||
| onClick={isClickable ? () => onViewDiff(result.resultId) : undefined} | ||
| onClick={ | ||
| isClickable ? () => onOpenResult(result.resultId, "diff") : undefined | ||
| } |
Contributor
There was a problem hiding this comment.
Make clickable rows keyboard-accessible.
onClick on <TableRow> makes this interaction pointer-only. Add keyboard activation (Enter/Space) and focusability for isClickable rows.
♿ Suggested fix
<TableRow
className={cn("hover:bg-muted/50", isClickable && "cursor-pointer")}
+ role={isClickable ? "button" : undefined}
+ tabIndex={isClickable ? 0 : undefined}
onClick={
isClickable ? () => onOpenResult(result.resultId, "diff") : undefined
}
+ onKeyDown={
+ isClickable
+ ? (e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ onOpenResult(result.resultId, "diff");
+ }
+ }
+ : undefined
+ }
>📝 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.
Suggested change
| <TableRow | |
| className={cn("hover:bg-muted/50", isClickable && "cursor-pointer")} | |
| onClick={isClickable ? () => onViewDiff(result.resultId) : undefined} | |
| onClick={ | |
| isClickable ? () => onOpenResult(result.resultId, "diff") : undefined | |
| } | |
| <TableRow | |
| className={cn("hover:bg-muted/50", isClickable && "cursor-pointer")} | |
| role={isClickable ? "button" : undefined} | |
| tabIndex={isClickable ? 0 : undefined} | |
| onClick={ | |
| isClickable ? () => onOpenResult(result.resultId, "diff") : undefined | |
| } | |
| onKeyDown={ | |
| isClickable | |
| ? (e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| onOpenResult(result.resultId, "diff"); | |
| } | |
| } | |
| : undefined | |
| } | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/app/routes/ws/deployments/page`.$deploymentId.plans.$planId.tsx
around lines 138 - 142, The TableRow currently uses onClick for navigation which
is pointer-only; make rows keyboard-accessible by, when isClickable is true,
adding focusability (e.g., tabIndex={0}), a proper semantic role (e.g.,
role="button" or adjust per semantics) and an onKeyDown handler that listens for
Enter and Space and calls onOpenResult(result.resultId, "diff") (for Space
ensure preventDefault to avoid scrolling). Update the TableRow props where
isClickable is used so it conditionally adds tabIndex, role, onKeyDown, and
keeps the existing onClick and cursor/focus styles so keyboard users can both
focus and activate the row.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit