Conversation
rschwietzke
added
java
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds optional Markdown rendering for report comments using a ::markdown:: prefix, styling the output in reports, and documenting the feature.
Changes:
- Add Markdown-to-HTML conversion for comment values prefixed with
::markdown::(case-insensitive) - Add flexmark-java dependencies and corresponding third-party license/notice entries
- Add report CSS styling and end-user documentation for Markdown comments
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/xceptance/xlt/report/providers/ConfigurationReportProvider.java | Adds Markdown processing for report comments via flexmark parser/renderer |
| src/test/java/com/xceptance/xlt/report/providers/ConfigurationReportProviderTest.java | Adds unit tests covering Markdown processing behavior |
| pom.xml | Adds flexmark-java dependencies for Markdown rendering |
| doc/feature-doc/markdown-comments.md | Documents how to use ::markdown:: comments and expected behavior |
| config/testreport/css/default.css | Adds styling rules for rendered Markdown blocks in reports |
| doc/3rd-party-licenses/flexmark-java/NOTICE | Adds flexmark-java notice |
| doc/3rd-party-licenses/flexmark-java/LICENSE | Adds flexmark-java license text |
| NOTICE.md | Registers flexmark-java in the project’s notice list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (cleanedString.substring(0, MARKDOWN_PREFIX.length()).equalsIgnoreCase(MARKDOWN_PREFIX)) | ||
| { | ||
| final String markdown = cleanedString.substring(MARKDOWN_PREFIX.length()); | ||
| final String html = MARKDOWN_RENDERER.render(MARKDOWN_PARSER.parse(markdown)); | ||
| return "<div class=\"markdown\">" + html + "</div>"; | ||
| } | ||
|
|
There was a problem hiding this comment.
cleanedString.substring(0, MARKDOWN_PREFIX.length()) will throw StringIndexOutOfBoundsException for any comment shorter than the prefix (e.g., "Hello World" or "Bold", both covered by the new tests). Also, the ::markdown::-only case will be treated as Markdown and wrapped in <div>...</div>, but the test expects it to be returned unchanged. Fix by checking length before slicing and by explicitly handling the “prefix only” case (length equals prefix length). Prefer regionMatches(true, ...) (or a safe length-guarded startsWith-style check) to avoid substringing just to compare.
| if (cleanedString.substring(0, MARKDOWN_PREFIX.length()).equalsIgnoreCase(MARKDOWN_PREFIX)) | |
| { | |
| final String markdown = cleanedString.substring(MARKDOWN_PREFIX.length()); | |
| final String html = MARKDOWN_RENDERER.render(MARKDOWN_PARSER.parse(markdown)); | |
| return "<div class=\"markdown\">" + html + "</div>"; | |
| } | |
| // If the cleaned string is shorter than the prefix, it cannot start with the prefix | |
| if (cleanedString.length() < MARKDOWN_PREFIX.length()) | |
| { | |
| return comment; | |
| } | |
| // Check for case-insensitive prefix match without creating a substring | |
| if (cleanedString.regionMatches(true, 0, MARKDOWN_PREFIX, 0, MARKDOWN_PREFIX.length())) | |
| { | |
| // If the string is exactly the prefix, treat it as a normal comment | |
| if (cleanedString.length() == MARKDOWN_PREFIX.length()) | |
| { | |
| return comment; | |
| } | |
| final String markdown = cleanedString.substring(MARKDOWN_PREFIX.length()); | |
| final String html = MARKDOWN_RENDERER.render(MARKDOWN_PARSER.parse(markdown)); | |
| return "<div class=\"markdown\">" + html + "</div>"; | |
| } |
| <!-- flexmark-java for Markdown-to-HTML conversion (report comments) --> | ||
| <dependency> | ||
| <groupId>com.vladsch.flexmark</groupId> | ||
| <artifactId>flexmark</artifactId> | ||
| <version>0.64.8</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.vladsch.flexmark</groupId> | ||
| <artifactId>flexmark-ext-tables</artifactId> | ||
| <version>0.64.8</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.vladsch.flexmark</groupId> | ||
| <artifactId>flexmark-ext-autolink</artifactId> | ||
| <version>0.64.8</version> |
There was a problem hiding this comment.
The flexmark version is duplicated across multiple dependencies, which makes upgrades/error-prone edits more likely. Consider extracting 0.64.8 into a Maven property (or dependencyManagement) and reference it from each dependency entry.
| | Input | Output | | ||
| |-------|--------| | ||
| | No prefix | Passed through as-is (raw HTML allowed) | | ||
| | `::markdown::` + content | Markdown → HTML, wrapped in `<div class="markdown">` | | ||
| | `::markdown::` only (no content) | Returned unchanged | | ||
| | `null` | Returned as `null` | |
There was a problem hiding this comment.
This table uses || at line starts, which typically renders as an extra empty column or malformed table in common Markdown renderers (including GFM). Replace with standard pipe table formatting (single leading/trailing |) so the “Behavior” table renders correctly.
| .markdown code { | ||
| background-color: #f0f0f0; | ||
| padding: 0.15em 0.4em; | ||
| border-radius: 3px; | ||
| font-size: 0.9em; | ||
| } | ||
| .markdown pre { | ||
| background-color: #f5f5f5; | ||
| border: 1px solid var(--main-border-color); |
There was a problem hiding this comment.
These new Markdown styles hardcode colors (#f0f0f0, #f5f5f5, and elsewhere #666), which may conflict with theming and contradict the doc’s claim that styling uses existing CSS custom properties for consistent appearance. Consider switching these to existing CSS variables (or introduce dedicated variables) so Markdown blocks adapt consistently across themes.
No description provided.