[WIP] Delete old plan comments on PR update #1943
Conversation
WalkthroughThe changes introduce several updates across documentation and backend code. The README.md received formatting improvements and expanded instructions, particularly for PostgreSQL usage and migration commands. In the backend, the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Database
participant Logger
User->>Database: GetDiggerJobsForPR(orgId, repoFullName, prNumber)
Database->>Database: Query DiggerBatch by repoFullName and prNumber
Database->>Logger: Log error if batch query fails
loop For each batch
Database->>Database: GetDiggerJobsForBatch(batchId)
Database->>Logger: Log error if job retrieval fails
end
Database->>Logger: Log summary info
Database-->>User: Return aggregated jobs or error
sequenceDiagram
participant Service
participant GitHubAPI
Service->>Service: DeleteComment(id string)
Service->>GitHubAPI: DeleteComment(int64(id))
GitHubAPI-->>Service: Success/Error
Service-->>Caller: Return error if any
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![greptile-apps[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces functionality to manage comment noise in pull requests by deleting old plan comments when PRs are updated, particularly beneficial for large monorepos.
- Added
DeleteCommentmethod ingithub.goto remove old plan comments via GitHub API - Introduced
PRCommentIdfield inDiggerJobstruct to track comment IDs for deletion - Added
GetDiggerJobsForPRfunction instorage.goto retrieve all jobs associated with a PR - Consider documenting when
PRCommentIdshould be null vs populated - Include
PRCommentIdinMapToJsonStruct()serialization for system-wide consistency
💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| _, err = svc.Client.Issues.DeleteComment(context.Background(), svc.Owner, svc.RepoName, commentId) | ||
| return err |
There was a problem hiding this comment.
style: Consider adding structured logging here to match the rest of the codebase's logging pattern
| allJobs := make([]DiggerJob, 0) | ||
| for _, batch := range batches { | ||
| jobs, err := db.GetDiggerJobsForBatch(batch.ID) | ||
| if err != nil { | ||
| slog.Error("error fetching digger jobs for batch", | ||
| "batchId", batch.ID, | ||
| "prNumber", prNumber, | ||
| "error", err) | ||
| return nil, err | ||
| } | ||
| allJobs = append(allJobs, jobs...) | ||
| } |
There was a problem hiding this comment.
style: Consider using a single query with JOIN to fetch all jobs instead of multiple queries in a loop to improve performance
![coderabbitai[bot]](https://cdn.statically.io/img/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)
README.md (1)
85-86: Improve grammar in the PostgreSQL SSL note.The phrasing could be improved for better clarity.
-Might need disabling ssl if running default docker image +You might need to disable SSL if running the default Docker image🧰 Tools
🪛 LanguageTool
[uncategorized] ~85-~85: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...irty ``` ## Local postgres Might need disabling ssl if running default docker image ``...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
backend/models/storage.go (1)
1842-1877: Well-implemented method to retrieve all jobs for a PR.The new
GetDiggerJobsForPRmethod effectively aggregates jobs across all batches for a specific PR, supporting the PR's objective of managing plan comments. The implementation includes proper error handling and detailed logging.Consider adding a limit or pagination option if PRs with many batches could lead to performance concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(5 hunks)backend/models/scheduler.go(2 hunks)backend/models/storage.go(1 hunks)libs/ci/github/github.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~85-~85: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...irty ``` ## Local postgres Might need disabling ssl if running default docker image ``...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
🪛 markdownlint-cli2 (0.17.2)
README.md
87-87: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (4)
backend/models/scheduler.go (1)
55-55: Added PRCommentId field to track comment associations.The addition of
PRCommentIdfield as a nullable*int64type allows for tracking which GitHub PR comments are associated with each digger job. This complements the new functionality to delete old plan comments on PR updates.libs/ci/github/github.go (1)
203-210: Good implementation of DeleteComment method following existing patterns.The implementation follows the same pattern as the existing
EditCommentmethod, with proper error handling for ID conversion and clear return of any GitHub API errors. This new method enables the deletion of old plan comments as described in the PR objectives.README.md (2)
80-81: Added important flag to migration command.Including the
--allow-dirtyflag in the migration command example provides better guidance for users running migrations.
88-88: Added useful example for DATABASE_URL with SSL disabled.This provides clear guidance for users working with local PostgreSQL instances, which is helpful for developers setting up their environment.
Currently on every PR update a bunch of new plan comments are appended to the PR. This creates a lot of noise, especially in large monorepos.
New behavior: delete old plan comments (not summaries!) before starting new batch of plan jobs
Summary by CodeRabbit
Documentation
New Features
Other Changes