Merge pull request #1944 from qodo-ai/tr/describe_Redesign

Tr/describe redesign

docs/docs/tools/describe.md

@@ -124,6 +124,10 @@ This option is enabled by default via the `pr_description.enable_pr_diagram` par
         <td><b>enable_semantic_files_types</b></td>
         <td>If set to true, "Changes walkthrough" section will be generated. Default is true.</td>
       </tr>
+      <tr>
+            <td><b>file_table_collapsible_open_by_default</b></td>
+            <td>If set to true, the file list in the "Changes walkthrough" section will be open by default. If set to false, it will be closed by default. Default is false.</td>
+      </tr>
       <tr>
         <td><b>collapsible_file_list</b></td>
         <td>If set to true, the file list in the "Changes walkthrough" section will be collapsible. If set to "adaptive", the file list will be collapsible only if there are more than 8 files. Default is "adaptive".</td>

pr_agent/algo/utils.py

@@ -70,7 +70,8 @@ class ReasoningEffort(str, Enum):
 
 
 class PRDescriptionHeader(str, Enum):
-    CHANGES_WALKTHROUGH = "### **Changes walkthrough** 📝"
+    DIAGRAM_WALKTHROUGH = "Diagram Walkthrough"
+    FILE_WALKTHROUGH = "File Walkthrough"
 
 
 def get_setting(key: str) -> Any:
@@ -1284,14 +1285,35 @@ def process_description(description_full: str) -> Tuple[str, List]:
     if not description_full:
         return "", []
 
-    description_split = description_full.split(PRDescriptionHeader.CHANGES_WALKTHROUGH.value)
+    # description_split = description_full.split(PRDescriptionHeader.FILE_WALKTHROUGH.value)
-    base_description_str = description_split[0]
+    if PRDescriptionHeader.FILE_WALKTHROUGH.value in description_full:
-    changes_walkthrough_str = ""
+        try:
-    files = []
+            # FILE_WALKTHROUGH are presented in a collapsible section in the description
-    if len(description_split) > 1:
+            regex_pattern = r'<details.*?>\s*<summary>\s*<h3>\s*' + re.escape(PRDescriptionHeader.FILE_WALKTHROUGH.value) + r'\s*</h3>\s*</summary>'
-        changes_walkthrough_str = description_split[1]
+            description_split = re.split(regex_pattern, description_full, maxsplit=1, flags=re.DOTALL)
+
+            # If the regex pattern is not found, fallback to the previous method
+            if len(description_split) == 1:
+                get_logger().debug("Could not find regex pattern for file walkthrough, falling back to simple split")
+                description_split = description_full.split(PRDescriptionHeader.FILE_WALKTHROUGH.value, 1)
+        except Exception as e:
+            get_logger().warning(f"Failed to split description using regex, falling back to simple split: {e}")
+            description_split = description_full.split(PRDescriptionHeader.FILE_WALKTHROUGH.value, 1)
+
+        if len(description_split) < 2:
+            get_logger().error("Failed to split description into base and changes walkthrough", artifact={'description': description_full})
+            return description_full.strip(), []
+
+        base_description_str = description_split[0].strip()
+        changes_walkthrough_str = ""
+        files = []
+        if len(description_split) > 1:
+            changes_walkthrough_str = description_split[1]
+        else:
+            get_logger().debug("No changes walkthrough found")
     else:
-        get_logger().debug("No changes walkthrough found")
+        base_description_str = description_full.strip()
+        return base_description_str, []
 
     try:
         if changes_walkthrough_str:
@@ -1314,18 +1336,20 @@ def process_description(description_full: str) -> Tuple[str, List]:
                 try:
                     if isinstance(file_data, tuple):
                         file_data = file_data[0]
-                    pattern = r'<details>\s*<summary><strong>(.*?)</strong>\s*<dd><code>(.*?)</code>.*?</summary>\s*<hr>\s*(.*?)\s*<li>(.*?)</details>'
+                    pattern = r'<details>\s*<summary><strong>(.*?)</strong>\s*<dd><code>(.*?)</code>.*?</summary>\s*<hr>\s*(.*?)\s*(?:<li>|•)(.*?)</details>'
                     res = re.search(pattern, file_data, re.DOTALL)
                     if not res or res.lastindex != 4:
                         pattern_back = r'<details>\s*<summary><strong>(.*?)</strong><dd><code>(.*?)</code>.*?</summary>\s*<hr>\s*(.*?)\n\n\s*(.*?)</details>'
                         res = re.search(pattern_back, file_data, re.DOTALL)
                     if not res or res.lastindex != 4:
-                        pattern_back = r'<details>\s*<summary><strong>(.*?)</strong>\s*<dd><code>(.*?)</code>.*?</summary>\s*<hr>\s*(.*?)\s*-\s*(.*?)\s*</details>' # looking for hyphen ('- ')
+                        pattern_back = r'<details>\s*<summary><strong>(.*?)</strong>\s*<dd><code>(.*?)</code>.*?</summary>\s*<hr>\s*(.*?)\s*-\s*(.*?)\s*</details>' # looking for hypen ('- ')
                         res = re.search(pattern_back, file_data, re.DOTALL)
                     if res and res.lastindex == 4:
                         short_filename = res.group(1).strip()
                         short_summary = res.group(2).strip()
                         long_filename = res.group(3).strip()
+                        if long_filename.endswith('<ul>'):
+                            long_filename = long_filename[:-4].strip()
                         long_summary =  res.group(4).strip()
                         long_summary = long_summary.replace('<br> *', '\n*').replace('<br>','').replace('\n','<br>')
                         long_summary = h.handle(long_summary).strip()
@@ -1344,7 +1368,7 @@ def process_description(description_full: str) -> Tuple[str, List]:
                         if '<code>...</code>' in file_data:
                             pass # PR with many files. some did not get analyzed
                         else:
-                            get_logger().error(f"Failed to parse description", artifact={'description': file_data})
+                            get_logger().warning(f"Failed to parse description", artifact={'description': file_data})
                 except Exception as e:
                     get_logger().exception(f"Failed to process description: {e}", artifact={'description': file_data})
 

pr_agent/git_providers/azuredevops_provider.py

@@ -380,7 +380,7 @@ class AzureDevopsProvider(GitProvider):
                 pr_body = pr_body[:ind]
 
             if len(pr_body) > MAX_PR_DESCRIPTION_AZURE_LENGTH:
-                changes_walkthrough_text = PRDescriptionHeader.CHANGES_WALKTHROUGH.value
+                changes_walkthrough_text = PRDescriptionHeader.FILE_WALKTHROUGH.value
                 ind = pr_body.find(changes_walkthrough_text)
                 if ind != -1:
                     pr_body = pr_body[:ind]

pr_agent/settings/configuration.toml

@@ -106,7 +106,7 @@ extra_instructions = ""
 enable_pr_type=true
 final_update_message = true
 enable_help_text=false
-enable_help_comment=true
+enable_help_comment=false
 enable_pr_diagram=true # adds a section with a diagram of the PR changes
 # describe as comment
 publish_description_as_comment=false

pr_agent/settings/pr_description_prompts.toml

@@ -48,8 +48,8 @@ class PRDescription(BaseModel):
     description: str = Field(description="summarize the PR changes in up to four bullet points, each up to 8 words. For large PRs, add sub-bullets if needed. Order bullets by importance, with each bullet highlighting a key change group.")
     title: str = Field(description="a concise and descriptive title that captures the PR's main theme")
 {%- if enable_pr_diagram %}
-    changes_diagram: str = Field(description="a horizontal diagram that represents the main PR changes, in the format of a valid mermaid LR flowchart. The diagram should be concise and easy to read. Leave empty if no diagram is relevant. To create robust Mermaid diagrams, follow this two-step process: (1) Declare the nodes: nodeID[\"node description\"]. (2) Then define the links: nodeID1 -- \"link text\" --> nodeID2. Node description must always be surrounded with quotation marks.")
+    changes_diagram: str = Field(description='a horizontal diagram that represents the main PR changes, in the format of a valid mermaid LR flowchart. The diagram should be concise and easy to read. Leave empty if no diagram is relevant. To create robust Mermaid diagrams, follow this two-step process: (1) Declare the nodes: nodeID["node description"]. (2) Then define the links: nodeID1 -- "link text" --> nodeID2. Node description must always be surrounded with double quotation marks')
-{%- endif %}
+'{%- endif %}
 {%- if enable_semantic_files_types %}
     pr_files: List[FileDescription] = Field(max_items=20, description="a list of all the files that were changed in the PR, and summary of their changes. Each file must be analyzed regardless of change size.")
 {%- endif %}
@@ -67,11 +67,11 @@ description: |
 title: |
   ...
 {%- if enable_pr_diagram %}
-  changes_diagram: |
+changes_diagram: |
-    ```mermaid
+  ```mermaid
-    flowchart LR
+  flowchart LR
-      ...
+    ...
-    ```
+  ```
 {%- endif %}
 {%- if enable_semantic_files_types %}
 pr_files:
@@ -155,11 +155,11 @@ description: |
 title: |
   ...
 {%- if enable_pr_diagram %}
-  changes_diagram: |
+changes_diagram: |
-    ```mermaid
+  ```mermaid
-    flowchart LR
+  flowchart LR
-      ...
+    ...
-    ```
+  ```
 {%- endif %}
 {%- if enable_semantic_files_types %}
 pr_files:

pr_agent/tools/pr_description.py

@@ -128,7 +128,7 @@ class PRDescription:
                 pr_title, pr_body, changes_walkthrough, pr_file_changes = self._prepare_pr_answer()
                 if not self.git_provider.is_supported(
                         "publish_file_comments") or not get_settings().pr_description.inline_file_summary:
-                    pr_body += "\n\n" + changes_walkthrough
+                    pr_body += "\n\n" + changes_walkthrough + "___\n\n"
             get_logger().debug("PR output", artifact={"title": pr_title, "body": pr_body})
 
             # Add help text if gfm_markdown is supported
@@ -331,7 +331,8 @@ class PRDescription:
             else:
                 original_prediction_dict = original_prediction_loaded
             if original_prediction_dict:
-                filenames_predicted = [file.get('filename', '').strip() for file in original_prediction_dict.get('pr_files', [])]
+                files = original_prediction_dict.get('pr_files', [])
+                filenames_predicted = [file.get('filename', '').strip() for file in files if isinstance(file, dict)]
             else:
                 filenames_predicted = []
 
@@ -555,15 +556,11 @@ class PRDescription:
         """
 
         # Iterate over the dictionary items and append the key and value to 'markdown_text' in a markdown format
-        markdown_text = ""
         # Don't display 'PR Labels'
         if 'labels' in self.data and self.git_provider.is_supported("get_labels"):
             self.data.pop('labels')
         if not get_settings().pr_description.enable_pr_type:
             self.data.pop('type')
-        for key, value in self.data.items():
-            markdown_text += f"## **{key}**\n\n"
-            markdown_text += f"{value}\n\n"
 
         # Remove the 'PR Title' key from the dictionary
         ai_title = self.data.pop('title', self.vars["title"])
@@ -579,6 +576,10 @@ class PRDescription:
         pr_body, changes_walkthrough = "", ""
         pr_file_changes = []
         for idx, (key, value) in enumerate(self.data.items()):
+            if key == 'changes_diagram':
+                pr_body += f"### {PRDescriptionHeader.DIAGRAM_WALKTHROUGH.value}\n\n"
+                pr_body += f"{value}\n\n"
+                continue
             if key == 'pr_files':
                 value = self.file_label_dict
             else:
@@ -597,9 +598,15 @@ class PRDescription:
                     pr_body += f'- `{filename}`: {description}\n'
                 if self.git_provider.is_supported("gfm_markdown"):
                     pr_body += "</details>\n"
-            elif 'pr_files' in key.lower() and get_settings().pr_description.enable_semantic_files_types:
+            elif 'pr_files' in key.lower() and get_settings().pr_description.enable_semantic_files_types: # 'File Walkthrough' section
-                changes_walkthrough, pr_file_changes = self.process_pr_files_prediction(changes_walkthrough, value)
+                changes_walkthrough_table, pr_file_changes = self.process_pr_files_prediction(changes_walkthrough, value)
-                changes_walkthrough = f"{PRDescriptionHeader.CHANGES_WALKTHROUGH.value}\n{changes_walkthrough}"
+                if get_settings().pr_description.get('file_table_collapsible_open_by_default', False):
+                    initial_status = " open"
+                else:
+                    initial_status = ""
+                changes_walkthrough = f"<details{initial_status}> <summary><h3> {PRDescriptionHeader.FILE_WALKTHROUGH.value}</h3></summary>\n\n"
+                changes_walkthrough += f"{changes_walkthrough_table}\n\n"
+                changes_walkthrough += "</details>\n\n"
             elif key.lower().strip() == 'description':
                 if isinstance(value, list):
                     value = ', '.join(v.rstrip() for v in value)
@@ -633,14 +640,19 @@ class PRDescription:
                                          artifact={"file": file})
                     continue
                 filename = file['filename'].replace("'", "`").replace('"', '`')
-                changes_summary = file.get('changes_summary', "").strip()
+                changes_summary = file.get('changes_summary', "")
+                if not changes_summary:
+                    get_logger().warning(f"Empty changes summary in file label dict, skipping file",
+                                         artifact={"file": file})
+                    continue
+                changes_summary = changes_summary.strip()
                 changes_title = file['changes_title'].strip()
                 label = file.get('label').strip().lower()
                 if label not in file_label_dict:
                     file_label_dict[label] = []
                 file_label_dict[label].append((filename, changes_title, changes_summary))
             except Exception as e:
-                get_logger().error(f"Error preparing file label dict {self.pr_id}: {e}")
+                get_logger().exception(f"Error preparing file label dict {self.pr_id}")
                 pass
         return file_label_dict
 
@@ -720,7 +732,7 @@ class PRDescription:
             pr_body += """</tr></tbody></table>"""
 
         except Exception as e:
-            get_logger().error(f"Error processing PR files to markdown {self.pr_id}: {str(e)}")
+            get_logger().error(f"Error processing pr files to markdown {self.pr_id}: {str(e)}")
             pass
         return pr_body, pr_comments
 
@@ -776,14 +788,21 @@ def insert_br_after_x_chars(text: str, x=70):
     if count_chars_without_html(text) < x:
         return text
 
+    is_list = text.lstrip().startswith(("- ", "* "))
+
     # replace odd instances of ` with <code> and even instances of ` with </code>
     text = replace_code_tags(text)
 
-    # convert list items to <li>
+    # convert list items to <li> only if the text is identified as a list
-    if text.startswith("- ") or text.startswith("* "):
+    if is_list:
-        text = "<li>" + text[2:]
+        # To handle lists that start with indentation
-    text = text.replace("\n- ", '<br><li> ').replace("\n - ", '<br><li> ')
+        leading_whitespace = text[:len(text) - len(text.lstrip())]
-    text = text.replace("\n* ", '<br><li> ').replace("\n * ", '<br><li> ')
+        body = text.lstrip()
+        body = "<li>" + body[2:]
+        text = leading_whitespace + body
+
+        text = text.replace("\n- ", '<br><li> ').replace("\n - ", '<br><li> ')
+        text = text.replace("\n* ", '<br><li> ').replace("\n * ", '<br><li> ')
 
     # convert new lines to <br>
     text = text.replace("\n", '<br>')
@@ -823,7 +842,13 @@ def insert_br_after_x_chars(text: str, x=70):
             is_inside_code = True
         if "</code>" in word:
             is_inside_code = False
-    return ''.join(new_text).strip()
+
+    processed_text = ''.join(new_text).strip()
+
+    if is_list:
+        processed_text = f"<ul>{processed_text}</ul>"
+
+    return processed_text
 
 
 def replace_code_tags(text):

tests/unittest/test_convert_to_markdown.py

@@ -51,7 +51,7 @@ class TestConvertToMarkdown:
         input_data = {'review': {
             'estimated_effort_to_review_[1-5]': '1, because the changes are minimal and straightforward, focusing on a single functionality addition.\n',
             'relevant_tests': 'No\n', 'possible_issues': 'No\n', 'security_concerns': 'No\n'}}
-        
+
         expected_output = textwrap.dedent(f"""\
             {PRReviewHeader.REGULAR.value} 🔍
 
@@ -67,12 +67,12 @@ class TestConvertToMarkdown:
         """)
 
         assert convert_to_markdown_v2(input_data).strip() == expected_output.strip()
-    
+
     def test_simple_dictionary_input_without_gfm_supported(self):
         input_data = {'review': {
             'estimated_effort_to_review_[1-5]': '1, because the changes are minimal and straightforward, focusing on a single functionality addition.\n',
             'relevant_tests': 'No\n', 'possible_issues': 'No\n', 'security_concerns': 'No\n'}}
-        
+
         expected_output = textwrap.dedent("""\
             ## PR Reviewer Guide 🔍
 
@@ -89,74 +89,74 @@ class TestConvertToMarkdown:
         """)
 
         assert convert_to_markdown_v2(input_data, gfm_supported=False).strip() == expected_output.strip()
-    
+
     def test_key_issues_to_review(self):
         input_data = {'review': {
             'key_issues_to_review': [
                 {
-                    'relevant_file' : 'src/utils.py',
+                    'relevant_file': 'src/utils.py',
-                    'issue_header' : 'Code Smell',
+                    'issue_header': 'Code Smell',
-                    'issue_content' : 'The function is too long and complex.',
+                    'issue_content': 'The function is too long and complex.',
                     'start_line': 30,
                     'end_line': 50,
                 }
             ]
         }}
         mock_git_provider = Mock()
-        reference_link = 'https://github.com/qodo/pr-agent/pull/1/files#diff-hashvalue-R174' 
+        reference_link = 'https://github.com/qodo/pr-agent/pull/1/files#diff-hashvalue-R174'
         mock_git_provider.get_line_link.return_value = reference_link
 
         expected_output = textwrap.dedent(f"""\
             ## PR Reviewer Guide 🔍
-                                          
+
             Here are some key observations to aid the review process:
-                                          
+
             <table>
             <tr><td>⚡&nbsp;<strong>Recommended focus areas for review</strong><br><br>
-                                                                                    
+
             <a href='{reference_link}'><strong>Code Smell</strong></a><br>The function is too long and complex.
-                                          
+
             </td></tr>
             </table>
         """)
-        
+
         assert convert_to_markdown_v2(input_data, git_provider=mock_git_provider).strip() == expected_output.strip()
         mock_git_provider.get_line_link.assert_called_with('src/utils.py', 30, 50)
-    
+
     def test_ticket_compliance(self):
         input_data = {'review': {
             'ticket_compliance_check': [
                 {
                     'ticket_url': 'https://example.com/ticket/123',
-                    'ticket_requirements': '- Requirement 1\n- Requirement 2\n', 
+                    'ticket_requirements': '- Requirement 1\n- Requirement 2\n',
-                    'fully_compliant_requirements': '- Requirement 1\n- Requirement 2\n', 
+                    'fully_compliant_requirements': '- Requirement 1\n- Requirement 2\n',
                     'not_compliant_requirements': '',
                     'requires_further_human_verification': '',
                 }
             ]
         }}
-        
+
         expected_output = textwrap.dedent("""\
             ## PR Reviewer Guide 🔍
-            
+
             Here are some key observations to aid the review process:
-                                          
+
             <table>
             <tr><td>
-                                          
+
             **🎫 Ticket compliance analysis ✅**
-                                          
+
-                                          
+
-                                          
+
             **[123](https://example.com/ticket/123) - Fully compliant**
-                                          
+
             Compliant requirements:
-                                          
+
             - Requirement 1
             - Requirement 2
-                                          
+
-                                          
+
-                                          
+
             </td></tr>
             </table>
         """)
@@ -179,43 +179,43 @@ class TestConvertToMarkdown:
                     ],
                     'title': 'Bug Fix',
                 }
-             ]
+            ]
-            }
+        }
         }
 
         expected_output = textwrap.dedent("""\
             ## PR Reviewer Guide 🔍
-                                          
+
             Here are some key observations to aid the review process:
 
             <table>
             <tr><td>🔀 <strong>Multiple PR themes</strong><br><br>
-                                          
+
             <details><summary>
             Sub-PR theme: <b>Refactoring</b></summary>
-                                          
+
             ___
-                                          
+
             Relevant files:
-                                          
+
             - src/file1.py
             - src/file2.py
             ___
-                                          
+
             </details>
-                                          
+
             <details><summary>
             Sub-PR theme: <b>Bug Fix</b></summary>
-                                          
+
             ___
-                                          
+
             Relevant files:
-                                          
+
             - src/file3.py
             ___
-                                          
+
             </details>
-                                          
+
             </td></tr>
             </table>
         """)
@@ -228,7 +228,6 @@ class TestConvertToMarkdown:
 
         expected_output = ''
 
-
         assert convert_to_markdown_v2(input_data).strip() == expected_output.strip()
 
     def test_dictionary_with_empty_dictionaries(self):
@@ -236,16 +235,16 @@ class TestConvertToMarkdown:
 
         expected_output = ''
 
-
         assert convert_to_markdown_v2(input_data).strip() == expected_output.strip()
 
+
 class TestBR:
     def test_br1(self):
         file_change_description = '- Imported `FilePatchInfo` and `EDIT_TYPE` from `pr_agent.algo.types` instead of `pr_agent.git_providers.git_provider`.'
         file_change_description_br = insert_br_after_x_chars(file_change_description)
-        expected_output = ('<li>Imported <code>FilePatchInfo</code> and <code>EDIT_TYPE</code> from '
+        expected_output = ('<ul><li>Imported <code>FilePatchInfo</code> and <code>EDIT_TYPE</code> from '
                            '<code>pr_agent.algo.types</code> instead <br>of '
-                           '<code>pr_agent.git_providers.git_provider</code>.')
+                           '<code>pr_agent.git_providers.git_provider</code>.</ul>')
         assert file_change_description_br == expected_output
         # print("-----")
         # print(file_change_description_br)
@@ -255,9 +254,9 @@ class TestBR:
             '- Created a - new -class `ColorPaletteResourcesCollection ColorPaletteResourcesCollection '
             'ColorPaletteResourcesCollection ColorPaletteResourcesCollection`')
         file_change_description_br = insert_br_after_x_chars(file_change_description)
-        expected_output = ('<li>Created a - new -class <code>ColorPaletteResourcesCollection </code><br><code>'
+        expected_output = ('<ul><li>Created a - new -class <code>ColorPaletteResourcesCollection </code><br><code>'
                            'ColorPaletteResourcesCollection ColorPaletteResourcesCollection '
-                           '</code><br><code>ColorPaletteResourcesCollection</code>')
+                           '</code><br><code>ColorPaletteResourcesCollection</code></ul>')
         assert file_change_description_br == expected_output
         # print("-----")
         # print(file_change_description_br)