Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe GitHub check title now reports aggregated unified-diff line counts (total additions and deletions across agents) and appends an errored count when present; prior count-based titles (changed/unchanged/unsupported) were removed except for the “All agents unsupported” special-case. Changes
Sequence Diagram(s)(Skipped — changes are localized and do not introduce a multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go (1)
57-74: Coverage gap: the new diff-counting logic isn't directly tested.The updated
TestAggregate_CheckTitlecases only assert on already-populatedAdditions/Deletionsvalues;countDiffLinesand theaggregateResultspath that populates those fields fromCurrent/Proposedaren't exercised. Consider extendingTestAggregateResults_Countsto assertagg.Additions/agg.Deletionsfor thecompletedResult("a", true, "old", "new")case, and/or adding a focusedTestCountDiffLineswith table cases (identical input → 0/0, pure additions, pure deletions, mixed, empty strings, content lines that happen to start with---or+++— which in a unified diff would appear as+---/-+++and must still be counted correctly).Also applies to: 140-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go` around lines 57 - 74, Extend TestAggregateResults_Counts to assert that aggregateResults populates Additions and Deletions for the completedResult("a", true, "old", "new") case (use agg.Additions and agg.Deletions) so countDiffLines is exercised, and/or add a new focused TestCountDiffLines table-driven test that calls countDiffLines with cases: identical inputs -> 0/0, pure additions, pure deletions, mixed changes, empty strings, and lines that begin with diff markers like "+---" or "-+++" to ensure those are counted correctly; locate the logic in aggregateResults and the countDiffLines function to wire inputs via completedResult helpers and assert expected numeric results.apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go (1)
157-182: Redundant unified-diff computation per changed agent.For every result with
HasChanges=true,difflib.GetUnifiedDiffStringis now invoked twice against the sameCurrent/Proposedinputs: once here withContext: 0to count lines, and again informatAgentSection(line 295) withContext: 3to render the markdown diff. For large manifests this doubles the diff work. Consider counting+/-lines directly from theContext: 3diff that's already produced informatAgentSectionand caching the totals onagentResult, or sharing a single diff through the aggregate pipeline.Also applies to: 295-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go` around lines 157 - 182, The countDiffLines function is redundantly recomputing a unified diff (Context:0) while formatAgentSection already generates a unified diff (Context:3); refactor so the unified diff is computed once and its +/− line counts are reused: have formatAgentSection (or the diff-producing step) return or attach the unified-diff string and computed adds/dels to the agentResult (e.g., add fields like AddedLines/DeletedLines or a CachedDiff string), compute adds/dels by scanning the already-produced Context:3 diff instead of calling difflib.GetUnifiedDiffString again, and update callers that currently call countDiffLines to read the cached totals; remove the duplicate GetUnifiedDiffString invocation in countDiffLines or inline its logic to use the cached diff.
🤖 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/workspace-engine/svc/controllers/deploymentplanresult/github_check.go`:
- Around line 264-269: The diff summary currently returns "+0 -0" when there are
no changes; update the logic in the formatting function that produces
diffSummary (the block using a.Additions, a.Deletions, a.Errored) to first
detect the all-clean case (a.Additions == 0 && a.Deletions == 0 && a.Changed ==
0) and return "No changes" in that case, otherwise build the "+%d -%d" summary
as before and still append the "(%d errored)" suffix when a.Errored > 0; keep
references to the same variables (a.Additions, a.Deletions, a.Changed,
a.Errored) so the change is a simple conditional early-return.
---
Nitpick comments:
In
`@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go`:
- Around line 57-74: Extend TestAggregateResults_Counts to assert that
aggregateResults populates Additions and Deletions for the completedResult("a",
true, "old", "new") case (use agg.Additions and agg.Deletions) so countDiffLines
is exercised, and/or add a new focused TestCountDiffLines table-driven test that
calls countDiffLines with cases: identical inputs -> 0/0, pure additions, pure
deletions, mixed changes, empty strings, and lines that begin with diff markers
like "+---" or "-+++" to ensure those are counted correctly; locate the logic in
aggregateResults and the countDiffLines function to wire inputs via
completedResult helpers and assert expected numeric results.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go`:
- Around line 157-182: The countDiffLines function is redundantly recomputing a
unified diff (Context:0) while formatAgentSection already generates a unified
diff (Context:3); refactor so the unified diff is computed once and its +/− line
counts are reused: have formatAgentSection (or the diff-producing step) return
or attach the unified-diff string and computed adds/dels to the agentResult
(e.g., add fields like AddedLines/DeletedLines or a CachedDiff string), compute
adds/dels by scanning the already-produced Context:3 diff instead of calling
difflib.GetUnifiedDiffString again, and update callers that currently call
countDiffLines to read the cached totals; remove the duplicate
GetUnifiedDiffString invocation in countDiffLines or inline its logic to use the
cached diff.
🪄 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: 6a1aec19-417f-4c96-83c3-f7d414e2bb70
📒 Files selected for processing (2)
apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.goapps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go
|
|
||
| diffSummary := fmt.Sprintf("+%d -%d", a.Additions, a.Deletions) | ||
| if a.Errored > 0 { | ||
| return fmt.Sprintf( | ||
| "%d errored, %d changed, %d unchanged, %d unsupported", | ||
| a.Errored, a.Changed, a.Unchanged, a.Unsupported, | ||
| ) | ||
| return fmt.Sprintf("%s (%d errored)", diffSummary, a.Errored) | ||
| } | ||
| if a.Changed > 0 { | ||
| return fmt.Sprintf( | ||
| "%d changed, %d unchanged, %d unsupported", | ||
| a.Changed, a.Unchanged, a.Unsupported, | ||
| ) | ||
| } | ||
| if a.Unsupported > 0 { | ||
| return fmt.Sprintf( | ||
| "No changes (%d unsupported)", | ||
| a.Unsupported, | ||
| ) | ||
| } | ||
| return "No changes" | ||
| return diffSummary |
There was a problem hiding this comment.
UX: "+0 -0" replaces the old "No changes" summary.
When every agent reports HasChanges=false, the title now reads +0 -0 (confirmed by the "final no changes shows zero diff counts" and "final all clean" test cases). That's technically correct, but arguably less informative than the prior "No changes" / human-readable summary. If intentional per the linked issue (#1033 asks specifically for line totals), feel free to ignore; otherwise consider falling back to "No changes" when a.Additions == 0 && a.Deletions == 0 && a.Changed == 0.
Optional fallback
diffSummary := fmt.Sprintf("+%d -%d", a.Additions, a.Deletions)
if a.Errored > 0 {
return fmt.Sprintf("%s (%d errored)", diffSummary, a.Errored)
}
+ if a.Additions == 0 && a.Deletions == 0 && a.Changed == 0 {
+ return "No changes"
+ }
return diffSummary📝 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.
| diffSummary := fmt.Sprintf("+%d -%d", a.Additions, a.Deletions) | |
| if a.Errored > 0 { | |
| return fmt.Sprintf( | |
| "%d errored, %d changed, %d unchanged, %d unsupported", | |
| a.Errored, a.Changed, a.Unchanged, a.Unsupported, | |
| ) | |
| return fmt.Sprintf("%s (%d errored)", diffSummary, a.Errored) | |
| } | |
| if a.Changed > 0 { | |
| return fmt.Sprintf( | |
| "%d changed, %d unchanged, %d unsupported", | |
| a.Changed, a.Unchanged, a.Unsupported, | |
| ) | |
| } | |
| if a.Unsupported > 0 { | |
| return fmt.Sprintf( | |
| "No changes (%d unsupported)", | |
| a.Unsupported, | |
| ) | |
| } | |
| return "No changes" | |
| return diffSummary | |
| diffSummary := fmt.Sprintf("+%d -%d", a.Additions, a.Deletions) | |
| if a.Errored > 0 { | |
| return fmt.Sprintf("%s (%d errored)", diffSummary, a.Errored) | |
| } | |
| if a.Additions == 0 && a.Deletions == 0 && a.Changed == 0 { | |
| return "No changes" | |
| } | |
| return diffSummary |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go`
around lines 264 - 269, The diff summary currently returns "+0 -0" when there
are no changes; update the logic in the formatting function that produces
diffSummary (the block using a.Additions, a.Deletions, a.Errored) to first
detect the all-clean case (a.Additions == 0 && a.Deletions == 0 && a.Changed ==
0) and return "No changes" in that case, otherwise build the "+%d -%d" summary
as before and still append the "(%d errored)" suffix when a.Errored > 0; keep
references to the same variables (a.Additions, a.Deletions, a.Changed,
a.Errored) so the change is a simple conditional early-return.
There was a problem hiding this comment.
Pull request overview
Updates the GitHub check run title for deployment plan results to show total line additions/deletions (per issue #1033) instead of changed/unchanged/unsupported counts.
Changes:
- Add diff line aggregation fields (
Additions,Deletions) to the resultaggregate. - Compute total added/deleted line counts across agents with changes and render as
+A -DincheckTitle(). - Update unit tests to assert the new title formatting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/workspace-engine/svc/controllers/deploymentplanresult/github_check.go | Adds diff line counting and updates check title formatting to show +additions -deletions. |
| apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go | Updates checkTitle test cases to match the new diff-count-based summary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ | ||
| A: difflib.SplitLines(current), | ||
| B: difflib.SplitLines(proposed), | ||
| FromFile: "current", | ||
| ToFile: "proposed", | ||
| Context: 0, | ||
| }) | ||
| if err != nil { | ||
| return 0, 0 | ||
| } | ||
| var adds, dels int | ||
| for line := range strings.SplitSeq(diff, "\n") { | ||
| if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") { | ||
| continue | ||
| } | ||
| if strings.HasPrefix(line, "+") { | ||
| adds++ | ||
| continue | ||
| } | ||
| if strings.HasPrefix(line, "-") { | ||
| dels++ | ||
| } | ||
| } |
There was a problem hiding this comment.
countDiffLines generates a full unified diff string and then scans it to count additions/deletions. This introduces a second diff computation per changed agent (the diff is already computed again in formatAgentSection when rendering output), which can materially increase CPU/memory use for large plans. Consider counting via difflib opcodes (without building a unified diff string), or computing the diff once per agent and reusing it for both the rendered diff and the line counts.
| diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ | |
| A: difflib.SplitLines(current), | |
| B: difflib.SplitLines(proposed), | |
| FromFile: "current", | |
| ToFile: "proposed", | |
| Context: 0, | |
| }) | |
| if err != nil { | |
| return 0, 0 | |
| } | |
| var adds, dels int | |
| for line := range strings.SplitSeq(diff, "\n") { | |
| if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") { | |
| continue | |
| } | |
| if strings.HasPrefix(line, "+") { | |
| adds++ | |
| continue | |
| } | |
| if strings.HasPrefix(line, "-") { | |
| dels++ | |
| } | |
| } | |
| currentLines := difflib.SplitLines(current) | |
| proposedLines := difflib.SplitLines(proposed) | |
| if len(currentLines) == 0 { | |
| return len(proposedLines), 0 | |
| } | |
| if len(proposedLines) == 0 { | |
| return 0, len(currentLines) | |
| } | |
| // Count additions/deletions without building a unified diff string. | |
| // The number of deleted lines is the number of lines in current that are | |
| // not part of the longest common subsequence (LCS); similarly for added | |
| // lines in proposed. This preserves line-based counting semantics while | |
| // avoiding the extra allocation and scan of a rendered diff. | |
| prev := make([]int, len(proposedLines)+1) | |
| curr := make([]int, len(proposedLines)+1) | |
| for i := 1; i <= len(currentLines); i++ { | |
| for j := 1; j <= len(proposedLines); j++ { | |
| if currentLines[i-1] == proposedLines[j-1] { | |
| curr[j] = prev[j-1] + 1 | |
| } else if prev[j] >= curr[j-1] { | |
| curr[j] = prev[j] | |
| } else { | |
| curr[j] = curr[j-1] | |
| } | |
| } | |
| prev, curr = curr, prev | |
| clear(curr) | |
| } | |
| lcs := prev[len(proposedLines)] | |
| adds := len(proposedLines) - lcs | |
| dels := len(currentLines) - lcs |
| adds, dels := countDiffLines(r.Current, r.Proposed) | ||
| a.Additions += adds | ||
| a.Deletions += dels |
There was a problem hiding this comment.
New behavior aggregates Additions/Deletions based on diffs, but the tests currently only assert the final title string and do not verify that aggregateResults computes Additions/Deletions correctly across multiple agents / multi-line changes. Add a focused unit test (and/or tests for countDiffLines) that asserts expected additions/deletions counts for representative diffs and ensures unchanged/unsupported/errored agents don’t affect the totals.
| adds, dels := countDiffLines(r.Current, r.Proposed) | |
| a.Additions += adds | |
| a.Deletions += dels | |
| if r.Status == db.DeploymentPlanTargetStatusCompleted { | |
| adds, dels := countDiffLines(r.Current, r.Proposed) | |
| a.Additions += adds | |
| a.Deletions += dels | |
| } |
fixes #1033
Summary by CodeRabbit
New Features
Tests