feat: show diff of current vs proposed rendered argo CR#1126
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds end-to-end support for including Argo CD Application custom resources in deployment plan diffs. The backend fetches live Applications, normalizes them for clean diffs, and includes them as distinct YAML sections. The frontend parses plan content into sections, manages tab/section selection via URL state, and renders section-specific diffs in a Monaco editor. ChangesPlan Results with Application CR and Section-aware Diffs
Sequence Diagram(s)sequenceDiagram
participant Plan as ArgoCDPlanner.Plan
participant AppGetter as ApplicationGetter
participant ManifestGetter as ManifestGetter
participant Normalizer as normalizeApplicationForDiff
Plan->>AppGetter: GetApplication(context, live app)
Plan->>ManifestGetter: GetCurrentManifests()
Plan->>ManifestGetter: GetProposedManifests()
AppGetter-->>Plan: Application or nil
ManifestGetter-->>Plan: current manifests
ManifestGetter-->>Plan: proposed manifests
Plan->>Normalizer: normalize proposed Application CR
alt live Application exists
Plan->>Normalizer: normalize current Application CR
end
Normalizer-->>Plan: normalized Application YAML(s)
Plan->>Plan: prepend Application CR(s) to manifest YAML bundles
Plan-->>Plan: return current & proposed with Application CRs included
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds ArgoCD Application CR visibility to deployment plan diffs and refactors the plan diff UI to support section-based viewing of the Application CR versus rendered manifests.
Changes:
- Adds ArgoCD Application fetching to the workspace-engine planner and includes normalized current/proposed Application CR YAML in plan diffs.
- Adds tests for Application CR inclusion and ArgoCD application fetch error handling.
- Refactors the web plan detail dialog/header and adds section selection for diff content.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.go | Registers the production Application getter with the ArgoCD planner. |
| apps/workspace-engine/pkg/jobagents/argo/argocd.go | Defines the ApplicationGetter interface. |
| apps/workspace-engine/pkg/jobagents/argo/argocd_plan.go | Fetches, normalizes, and includes Application CR YAML in plan diffs. |
| apps/workspace-engine/pkg/jobagents/argo/argoapp_test.go | Updates planner tests and adds Application CR diff coverage. |
| apps/workspace-engine/pkg/jobagents/argo/application_upserter.go | Implements production Application fetching through the ArgoCD API. |
| apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx | Refactors page header/dialog ownership and uses URL-backed result opening. |
| apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts | Adds URL-backed tab state for plan result dialogs. |
| apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx | Refactors diff/validation dialog and adds section-aware diff rendering. |
| apps/web/app/routes/ws/deployments/_components/plans/PlanDetailPageHeader.tsx | Extracts the plan detail breadcrumb/header. |
| apps/web/app/routes/ws/deployments/_components/plans/plan-diff/SectionSelector.tsx | Adds the section dropdown for diff sections. |
| apps/web/app/routes/ws/deployments/_components/plans/plan-diff/DiffEditorPane.tsx | Extracts the Monaco diff editor pane. |
| apps/web/app/lib/plan-sections.ts | Adds YAML stream parsing and section extraction helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+34
| const cr = docs.filter(isArgoApplication); | ||
| const manifests = docs.filter((d) => !isArgoApplication(d)); |
Comment on lines
+39
to
+41
| const diffQuery = trpc.deployment.plans.resultDiff.useQuery( | ||
| { deploymentId, resultId }, | ||
| { enabled: open }, | ||
| { deploymentId: deployment.id, resultId: resultId ?? "" }, | ||
| { enabled: resultId != null }, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/web/app/routes/ws/deployments/_components/plans/plan-diff/DiffEditorPane.tsx (1)
5-13: ⚡ Quick winPromote exported props shape to an
interface.For exported component APIs, define a named props
interfaceinstead of an inline object type.Proposed change
+interface DiffEditorPaneProps { + current: string; + proposed: string; + view: "split" | "unified"; +} + export function DiffEditorPane({ current, proposed, view, -}: { - current: string; - proposed: string; - view: "split" | "unified"; -}) { +}: DiffEditorPaneProps) {As per coding guidelines,
**/*.{ts,tsx}: "Use TypeScript with explicit types; preferinterfacefor public APIs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/routes/ws/deployments/_components/plans/plan-diff/DiffEditorPane.tsx` around lines 5 - 13, Export a named props interface for the DiffEditorPane component instead of the inline object type: create and export an interface (e.g., DiffEditorPaneProps) that declares current: string; proposed: string; view: "split" | "unified"; then update the DiffEditorPane declaration to accept props: DiffEditorPaneProps (and keep existing destructuring of current, proposed, view). Ensure the new interface is exported so the component's public API uses an explicit, named type.apps/web/app/lib/plan-sections.ts (1)
3-3: ⚡ Quick winUse
interfacefor exportedPlanSection.
PlanSectionis an exported object contract; use aninterfaceto align with project API typing conventions.Proposed change
-export type PlanSection = { name: string; content: string }; +export interface PlanSection { + name: string; + content: string; +}As per coding guidelines,
**/*.{ts,tsx}: "Use TypeScript with explicit types; preferinterfacefor public APIs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/lib/plan-sections.ts` at line 3, Replace the exported type alias PlanSection with an exported interface to follow the project's public API typing convention: change the declaration of PlanSection (currently "export type PlanSection = { name: string; content: string }") to an exported interface named PlanSection with the same properties (name: string; content: string) so callers see the interface contract instead of a type alias.apps/web/app/routes/ws/deployments/_components/plans/plan-diff/SectionSelector.tsx (1)
9-13: ⚡ Quick winUse
interfacefor exportedSectionSelectorProps.Switch this exported object contract from
typetointerfacefor API consistency.Proposed change
-export type SectionSelectorProps = { +export interface SectionSelectorProps { sections: string[]; value: string | undefined; onChange: (value: string) => void; -}; +}As per coding guidelines,
**/*.{ts,tsx}: "Use TypeScript with explicit types; preferinterfacefor public APIs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/routes/ws/deployments/_components/plans/plan-diff/SectionSelector.tsx` around lines 9 - 13, Replace the exported type alias with an exported interface: change the declaration "export type SectionSelectorProps = { sections: string[]; value: string | undefined; onChange: (value: string) => void; }" to an equivalent "export interface SectionSelectorProps { sections: string[]; value?: string; onChange(value: string): void; }" (or keep value: string | undefined if you prefer explicit undefined), updating the symbols SectionSelectorProps, sections, value, and onChange accordingly to maintain the same shape and exported API.apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx (1)
44-47: ⚡ Quick winAvoid reparsing YAML streams on every render.
unionSectionNames(...)parses both streams, andDiffBodyparses them again. Memoize extracted sections once perdiffQuery.dataand reuse for names + selected section lookup.Also applies to: 143-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx` around lines 44 - 47, Memoize parsing of the YAML streams so you don't reparse on every render: use useMemo keyed on diffQuery.data (or its current/proposed fields) to produce parsedSections for both current and proposed, then call unionSectionNames(parsedSections.current, parsedSections.proposed) instead of unionSectionNames(diffQuery.data?.current, ...), and pass the parsedSections into DiffBody and any selected-section lookup logic (e.g. where you derive selectedSection by name) so both the names list and the selected-section resolution reuse the same parsed result rather than reparsing; update references to unionSectionNames, DiffBody, and the selected section lookup to use the memoized parsedSections.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/app/lib/plan-sections.ts`:
- Line 3: Replace the exported type alias PlanSection with an exported interface
to follow the project's public API typing convention: change the declaration of
PlanSection (currently "export type PlanSection = { name: string; content:
string }") to an exported interface named PlanSection with the same properties
(name: string; content: string) so callers see the interface contract instead of
a type alias.
In
`@apps/web/app/routes/ws/deployments/_components/plans/plan-diff/DiffEditorPane.tsx`:
- Around line 5-13: Export a named props interface for the DiffEditorPane
component instead of the inline object type: create and export an interface
(e.g., DiffEditorPaneProps) that declares current: string; proposed: string;
view: "split" | "unified"; then update the DiffEditorPane declaration to accept
props: DiffEditorPaneProps (and keep existing destructuring of current,
proposed, view). Ensure the new interface is exported so the component's public
API uses an explicit, named type.
In
`@apps/web/app/routes/ws/deployments/_components/plans/plan-diff/SectionSelector.tsx`:
- Around line 9-13: Replace the exported type alias with an exported interface:
change the declaration "export type SectionSelectorProps = { sections: string[];
value: string | undefined; onChange: (value: string) => void; }" to an
equivalent "export interface SectionSelectorProps { sections: string[]; value?:
string; onChange(value: string): void; }" (or keep value: string | undefined if
you prefer explicit undefined), updating the symbols SectionSelectorProps,
sections, value, and onChange accordingly to maintain the same shape and
exported API.
In `@apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx`:
- Around line 44-47: Memoize parsing of the YAML streams so you don't reparse on
every render: use useMemo keyed on diffQuery.data (or its current/proposed
fields) to produce parsedSections for both current and proposed, then call
unionSectionNames(parsedSections.current, parsedSections.proposed) instead of
unionSectionNames(diffQuery.data?.current, ...), and pass the parsedSections
into DiffBody and any selected-section lookup logic (e.g. where you derive
selectedSection by name) so both the names list and the selected-section
resolution reuse the same parsed result rather than reparsing; update references
to unionSectionNames, DiffBody, and the selected section lookup to use the
memoized parsedSections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3cfa9b5-80eb-4151-8638-fd66196eb6d1
📒 Files selected for processing (12)
apps/web/app/lib/plan-sections.tsapps/web/app/routes/ws/deployments/_components/plans/PlanDetailPageHeader.tsxapps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsxapps/web/app/routes/ws/deployments/_components/plans/plan-diff/DiffEditorPane.tsxapps/web/app/routes/ws/deployments/_components/plans/plan-diff/SectionSelector.tsxapps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.tsapps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsxapps/workspace-engine/pkg/jobagents/argo/application_upserter.goapps/workspace-engine/pkg/jobagents/argo/argoapp_test.goapps/workspace-engine/pkg/jobagents/argo/argocd.goapps/workspace-engine/pkg/jobagents/argo/argocd_plan.goapps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.go
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.
fixes #1075
fixes #1076
Summary by CodeRabbit
New Features
Refactor