feat: dry run commit check links back to individual plan result#1038
feat: dry run commit check links back to individual plan result#1038adityachoudhari26 merged 2 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 17 seconds. ⌛ 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)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Updates the GitHub check run “Details” URL (and the summary link) so it deep-links into the Ctrlplane UI for a specific deployment plan result, addressing issue #1031.
Changes:
- Update plan “Details” URL construction to use
resultId(instead oftarget) so the UI can open the diff dialog for a specific result. - Thread
resultIDthrough check output building and check run upsert. - Update unit test call site for the
buildCheckOutputsignature change.
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 | Builds check run details/summary URLs using resultId and threads resultID through output + upsert. |
| apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go | Updates buildCheckOutput invocation to include a resultID. |
Comments suppressed due to low confidence (1)
apps/workspace-engine/svc/controllers/deploymentplanresult/github_check_test.go:281
- This test was updated to pass a
resultID, but it doesn't assert that the generated summary URL actually contains theresultIdquery param (the new behavior in this PR). Use a deterministic UUID in the test and add an assertion that*out.SummaryincludesresultId=<uuid>so regressions are caught.
out := buildCheckOutput(tc, uuid.New(), results, agg)
require.NotNil(t, out)
require.NotNil(t, out.Title)
require.NotNil(t, out.Summary)
require.NotNil(t, out.Text)
assert.Contains(t, *out.Summary, "v1.2.3")
assert.Contains(t, *out.Summary, "/acme/deployments/")
assert.Contains(t, *out.Summary, tc.PlanID.String())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // targetDetailsURL returns the ctrlplane UI link for a specific result | ||
| // within a plan (used as the check's "Details" link). The URL opens the | ||
| // diff dialog for that result on page load. | ||
| func targetDetailsURL(ctx targetContext, resultID uuid.UUID) string { |
There was a problem hiding this comment.
targetDetailsURL now builds a URL keyed by resultID (and uses resultId query param), so the helper name no longer matches what it returns. Consider renaming the function (and call sites) to something like resultDetailsURL/planResultDetailsURL to avoid future confusion.
| var summary strings.Builder | ||
| fmt.Fprintf(&summary, "**Version:** `%s`\n\n", tc.VersionTag) | ||
| fmt.Fprintf(&summary, "[View full plan →](%s)\n", targetDetailsURL(tc)) | ||
| fmt.Fprintf(&summary, "[View full plan →](%s)\n", targetDetailsURL(tc, resultID)) |
There was a problem hiding this comment.
The summary link text still says "View full plan" but the URL now includes resultId=... and (per the comment above) will open the diff dialog for a specific result on page load. Update the link label to match the new behavior (e.g., "View result" / "View diff"), so users aren't surprised by the deep-link.
| fmt.Fprintf(&summary, "[View full plan →](%s)\n", targetDetailsURL(tc, resultID)) | |
| fmt.Fprintf(&summary, "[View diff →](%s)\n", targetDetailsURL(tc, resultID)) |
fixes #1031