fix: harden review module flows and repair mention/reminder consistency#241
fix: harden review module flows and repair mention/reminder consistency#241
Conversation
This comment was marked as spam.
This comment was marked as spam.
Summary of ChangesHello @vLuckyyy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly hardens the GitHub review module by introducing robust data persistence for review mentions and reminders, ensuring consistent state tracking of pull requests. It refines the handling of GitHub API rate limits for reminder dispatches and enhances the reliability of review-related commands through improved input validation and error handling. These changes collectively contribute to a more stable and accurate review process within the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly hardens the review module by improving asynchronous flows, fixing consistency issues with mentions and reminders, and adding persistence for review states. The changes are well-structured and address important aspects like rate limiting and data validation. My review includes suggestions to further enhance efficiency by reducing redundant API calls, improve the robustness of asynchronous operations, and refine the database schema for better maintainability and performance. Overall, this is a quality contribution that makes the review feature much more reliable.
| CompletableFuture<Void> chain = CompletableFuture.completedFuture(null); | ||
| for (GitHubReviewMentionRepository.ReviewerReminder reminder : reminders) { | ||
| chain = chain | ||
| .thenRun(() -> this.sendReminder(reminder)) | ||
| .thenCompose(unused -> this.delay(GITHUB_API_RATE_LIMIT)); | ||
| } |
There was a problem hiding this comment.
The current implementation of the asynchronous chain uses thenRun, which executes sendReminder but doesn't wait for its internal asynchronous operations (like JDA's .queue()) to complete. This could lead to overlapping operations and incorrect timing for reminders.
To make the chain more robust, I suggest refactoring sendReminder to return a CompletableFuture<Void> that completes only when all its work is done. Then, you can use thenCompose here to properly chain the asynchronous operations.
| CompletableFuture<Void> chain = CompletableFuture.completedFuture(null); | |
| for (GitHubReviewMentionRepository.ReviewerReminder reminder : reminders) { | |
| chain = chain | |
| .thenRun(() -> this.sendReminder(reminder)) | |
| .thenCompose(unused -> this.delay(GITHUB_API_RATE_LIMIT)); | |
| } | |
| CompletableFuture<Void> chain = CompletableFuture.completedFuture(null); | |
| for (GitHubReviewMentionRepository.ReviewerReminder reminder : reminders) { | |
| chain = chain | |
| .thenCompose(unused -> this.sendReminder(reminder)) | |
| .thenCompose(unused -> this.delay(GITHUB_API_RATE_LIMIT)); | |
| } |
| boolean isMerged = GitHubReviewUtil.isPullRequestMerged(pullRequest, this.appConfig.githubToken); | ||
| boolean isClosed = !isMerged && | ||
| GitHubReviewUtil.isPullRequestClosed(pullRequest, this.appConfig.githubToken); |
There was a problem hiding this comment.
These two consecutive calls to isPullRequestMerged and isPullRequestClosed each make a separate API request to fetch the pull request details. This is inefficient.
To optimize this, I recommend creating a new method in GitHubReviewUtil that fetches the pull request data once and returns an object or JsonObject containing both the merged and closed states. This would halve the number of API calls in this section.
| }).exceptionally(throwable -> { | ||
| LOGGER.log(Level.SEVERE, "Exception in isMentioned", throwable); | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
The exception handling in this exceptionally block is inconsistent with other parts of the codebase. While the exception is logged, it's not reported to Sentry, unlike in the catch block above and other places in the application. For consistent error monitoring, exceptions caught here should also be sent to Sentry. This applies to other exceptionally blocks in this file as well (e.g., in getReviewersNeedingReminders, find, updateReviewStatus).
}).exceptionally(throwable -> {
Sentry.captureException(throwable);
LOGGER.log(Level.SEVERE, "Exception in isMentioned", throwable);
return false;
});| .raw("(lastReminderSent IS NULL OR lastReminderSent = 0 OR lastReminderSent < " | ||
| + cutoffTimeMillis + ")") |
There was a problem hiding this comment.
Using where().raw() with string concatenation for building SQL queries can be risky and less readable. Although there's no direct SQL injection vulnerability here since cutoffTimeMillis is a long, it's better to use the ORMLite query builder's methods for type safety and clarity.
Consider rewriting this condition using where.isNull(), where.eq(), and where.lt() combined with where.or().
.and()
.or(
new Where<GitHubReviewMentionWrapper, String>().isNull("lastReminderSent"),
new Where<GitHubReviewMentionWrapper, String>().eq("lastReminderSent", 0L),
new Where<GitHubReviewMentionWrapper, String>().lt("lastReminderSent", cutoffTimeMillis)
)|
|
||
| private static final String KEY_SEPARATOR = "|"; | ||
|
|
||
| @DatabaseField(id = true) |
There was a problem hiding this comment.
Using a single string field as a primary key by concatenating the pull request URL and user ID (e.g., url|userId) is not ideal for database design. It makes querying less efficient (requiring LIKE operations as seen in the repository implementation) and harder to maintain.
A better approach would be to use a composite primary key with two separate columns: one for the pull request URL and one for the user ID. ORMLite supports composite keys. This would lead to a cleaner schema and more efficient queries.
1 participant
No description provided.