-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(review): remove stale progress comments #2788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,7 @@ def parse_incremental(self, args: List[str]): | |
|
|
||
| async def run(self) -> None: | ||
| init_run_details() | ||
| progress_response = None | ||
| try: | ||
| if not self.git_provider.get_files(): | ||
| get_logger().info(f"PR has no files: {self.pr_url}, skipping review") | ||
|
|
@@ -175,11 +176,10 @@ async def run(self) -> None: | |
| return None | ||
|
|
||
| if get_settings().config.publish_output and not get_settings().config.get('is_auto_command', False): | ||
| self.git_provider.publish_comment("Preparing review...", is_temporary=True) | ||
| progress_response = self.git_provider.publish_comment("Preparing review...", is_temporary=True) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checked what every provider returns here against its
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That matches what I had in mind:
In the future, we could standardize the comment handle across providers, but that feels separate from this PR. |
||
|
|
||
| await retry_with_fallback_models(self._prepare_prediction, model_type=ModelType.REGULAR) | ||
| if not self.prediction: | ||
| self.git_provider.remove_initial_comment() | ||
| return None | ||
|
|
||
| pr_review = self._prepare_pr_review() | ||
|
|
@@ -207,12 +207,16 @@ async def run(self) -> None: | |
| **review_thread_kwargs) | ||
| else: | ||
| self.git_provider.publish_comment(pr_review, **review_thread_kwargs) | ||
|
|
||
| self.git_provider.remove_initial_comment() | ||
| except Exception as e: | ||
| get_logger().error(f"Failed to review PR: {e}") | ||
| if get_settings().config.get("propagate_tool_errors", False): | ||
| raise | ||
| finally: | ||
| if progress_response is not None: | ||
| try: | ||
| self.git_provider.remove_comment(progress_response) | ||
| except Exception as e: | ||
| get_logger().exception(f"Failed to remove review progress comment, error: {e}") | ||
|
Comment on lines
+214
to
+219
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right shape, and I prefer it to the One thing worth confirming as deliberate: a failed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that a visible failure result could be better for manual runs. |
||
|
|
||
| def _should_publish_review_no_suggestions(self, pr_review: str) -> bool: | ||
| return get_settings().pr_reviewer.get('publish_output_no_suggestions', True) or "No major issues detected" not in pr_review | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both shapes are genuinely live, so this needs to stay tolerant:
remove_initial_commentstill feeds it the bare id out oftemp_comments, while the new caller passes the whole dict.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I kept it compatible with both forms. We could normalize the handle later, but I think changing the provider boundary belongs in a separate PR.