feat(improve): make suggestions heading configurable - #2820
Conversation
PR Summary by QodoMake /improve headings configurable with stable comment identity
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1.
|
|
Code review by qodo was updated up to the latest commit 23dc87d |
23dc87d to
0167e09
Compare
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 0167e09 |
|
Code review by qodo was updated up to the latest commit 1272b46 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Approving, with small things inline.
Picking the anchor by anchor_time across the combined tuple is better than the fix I was going to suggest. Yours also handles a persistent comment that was edited more recently than a newer note was created. The capability method is the right shape for the LocalGit branch too.
One thing to drop before merge: the three .pr_agent.toml lines. That file's own header says it feeds Qodo's hosted agent, not the pr-agent in this repo, and the value is the same as the default anyway. #2823 needs the same three lines removed, and it still conflicts with this PR in algo/utils.py.
Thanks for building the hidden anchor from the issue rather than a plain config key, and for turning both bot findings round this fast.
| [pr_code_suggestions] | ||
| suggestions_heading = "PR Code Suggestions" | ||
|
|
There was a problem hiding this comment.
This file's header says it is read by Qodo's hosted agent, not by the pr-agent in this repo, so the rule that asked for the sync does not apply here. The value is an exact copy of the default in configuration.toml as well. #2797 left it alone for the same reason. Worth dropping the same three lines from #2823.
| [pr_code_suggestions] | |
| suggestions_heading = "PR Code Suggestions" |
| @@ -2,7 +2,11 @@ | |||
|
|
|||
| PR_HEADER_START_WITH = '### **User description**\nupdate cli_pip.py\n\n\n___\n\n### **PR Type**' | |||
| REVIEW_START_WITH = '## PR Reviewer Guide 🔍\n\n<table>\n<tr><td>⏱️ <strong>Estimated effort to review</strong>:' | |||
There was a problem hiding this comment.
While you are here: part 1 added the review identity marker but not the matching prefix here, so this constant no longer matches a published review. Nothing catches it, because e2e_tests.yaml only runs on workflow_dispatch.
| REVIEW_START_WITH = '## PR Reviewer Guide 🔍\n\n<table>\n<tr><td>⏱️ <strong>Estimated effort to review</strong>:' | |
| REVIEW_START_WITH = '## PR Reviewer Guide 🔍\n\n<!-- pr-agent:review:full -->\n\n<table>\n<tr><td>⏱️ <strong>Estimated effort to review</strong>:' |
| ) | ||
| else: | ||
| # get the text of the previous suggestions until the latest commit | ||
| sections = prev_suggestions.split(history_header.strip()) |
There was a problem hiding this comment.
Nothing in tests/unittest reaches this branch. Coverage says lines 487-512 are never executed by the file's 45 tests, yet this is the normal state from the second /improve onward.
I ran it by hand and the migration is correct, so this is a gap rather than a bug. I have put a test for it in a suggestion at the end of the test file.
|
|
||
| provider.edit_comment.assert_not_called() | ||
| published = provider.publish_comment.call_args.args[0] | ||
| assert PRCodeSuggestionsIdentity.SUMMARY.value in published |
There was a problem hiding this comment.
A test for the branch above, if you want it. It builds a persistent comment that already has a #### Previous suggestions section and checks the custom heading and the marker survive, and that both the old and the newly folded entry are kept.
I ran it against your branch merged onto today's main: 46 pass. With it, the uncovered range at 487-512 drops to a single line, 501, which is the max_previous_comments eviction and wants its own case.
| assert PRCodeSuggestionsIdentity.SUMMARY.value in published | |
| assert PRCodeSuggestionsIdentity.SUMMARY.value in published | |
| def test_custom_heading_is_kept_when_a_history_section_already_exists(): | |
| existing = MagicMock() | |
| existing.body = ( | |
| "## Previous Custom Heading ✨\n\n" | |
| f"{PRCodeSuggestionsIdentity.SUMMARY.value}\n\n" | |
| "<!-- aaa1111 -->\n\n" | |
| "Latest suggestions up to commit aaa1111\n\n" | |
| "<table>latest</table>\n\n___\n\n" | |
| "#### Previous suggestions\n" | |
| "<details><summary>Suggestions up to commit 0000000</summary>\n" | |
| "<br><table>older</table>\n\n</details>\n" | |
| ) | |
| provider = _persistent_provider([existing]) | |
| custom_header = "## Latest Custom Heading ✨" | |
| result = PRCodeSuggestions.publish_persistent_comment_with_history( | |
| provider, | |
| f"{custom_header}\n\n<table>new suggestions</table>", | |
| initial_header=custom_header, | |
| name="suggestions", | |
| identity_marker=PRCodeSuggestionsIdentity.SUMMARY.value, | |
| legacy_initial_header=PRCodeSuggestionsHeader.SUMMARY.value, | |
| ) | |
| assert result is existing | |
| updated = provider.edit_comment.call_args.args[1] | |
| assert updated.startswith( | |
| f"{custom_header}\n\n{PRCodeSuggestionsIdentity.SUMMARY.value}\n\n<!-- deadbee -->" | |
| ) | |
| assert "Suggestions up to commit aaa1111" in updated | |
| assert "Suggestions up to commit 0000000" in updated | |
| provider.publish_comment.assert_not_called() |
1272b46 to
554bc16
Compare
|
Code review by qodo was updated up to the latest commit 554bc16 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Re-approving on the rebase. All three taken, and thanks for adding the test rather than just noting it.
Suite is green on your head merged onto today's main. Ready from my side.
Only thing left is ordering against #2823, which touches the same two files. No action needed unless it lands first.
2 participants
Related to #2038 (Part 2)
Summary
pr_code_suggestions.suggestions_headingfor summary-table/improveoutputimprove.mdFor example,
suggestions_heading = "Guideline Improvement Suggestions"renders## Guideline Improvement Suggestions ✨while continuing to update the same persistent summary.Testing