Addition of Tulip-Reporting module for Kotlin based report generation.#490
Open
bmvanwyk wants to merge 72 commits into
Open
Addition of Tulip-Reporting module for Kotlin based report generation.#490bmvanwyk wants to merge 72 commits into
bmvanwyk wants to merge 72 commits into
Conversation
- Updated ReportScripts to improve chart rendering with enhanced options for tooltips, legends, and series toggling. - Introduced a new download feature for charts in ReportScripts. - Refined ReportStyles with a new color scheme and improved layout for sidebar and main content. - Added responsive design elements to the report layout, including a fixed sidebar and flexible main content area. - Enhanced TulipReportGenerator to include a logo and improved metadata display. - Updated HTML structure to include navigation links for benchmarks and configuration sections. - Improved test cases in TulipReportGeneratorTest to validate new report structure and content.
- Migrated reporting logic from legacy Jython 2.7 to native Kotlin using kotlinx.html. - Implemented a modern 'Grafana-style' dark theme with Tulip brand identity. - Added hierarchical summary and detailed action results tables with purplish highlights. - Enhanced Google Charts with linear 50-100% percentile scaling and 5% steps. - Enabled interactive features: drag-to-zoom, right-click reset, and legend toggling. - Added chart export functionality (Save as Image) for all visualizations. - Integrated sidebar navigation with scroll-spy to highlight active sections. - Unified benchmark configuration and captured JVM runtime info into the main report. - Ensured full UI responsiveness with mobile-friendly layouts and scrollable tables. - Implemented robust JSON parsing with HdrHistogram aggregation across iterations.
…tt#445) - BenchmarkStats.kt: Created this new file to house serializable data classes (ActionStatResult, BenchmarkResult, JavaInfo, BenchmarkHeader) and helper functions for JVM information. - ActionStats.kt: Replaced the manual toJson method with toActionStatResult, providing a clean object representation of action statistics. - DataCollector.kt: Refactored saveStatsJson to use the new data classes and Json.encodeToString for both the file header and individual benchmark results. - TulipApi.java: Simplified getJavaInformation by delegating to a new Kotlin helper, eliminating complex manual JSON construction in Java. Verified the changes by running a benchmark test and confirming that the generated benchmark_output.json is valid and correctly formatted. Co-authored-by: Werner Fouché <werner.fouche@gmail.com>
- Introduced ReportStyles.kt for consistent styling in reports. - Implemented TulipReport.kt to handle HTML report creation from JSON input. - Developed TulipReportGenerator.kt for generating HTML and AsciiDoc reports, including detailed charts and metrics. - Added unit tests for report components and report generation to ensure functionality and correctness.
… report creation - Deleted `ReportStyles.kt` as it was no longer needed. - Refactored `TulipReport.kt` to directly use base64 encoded Python scripts for report generation, removing the dependency on JSON files. - Removed `TulipReportGenerator.kt` and its associated methods, consolidating report generation logic into `TulipReport.kt`. - Deleted test files `ReportComponentsTest.kt`, `ReportDataTest.kt`, and `TulipReportGeneratorTest.kt` as they were tied to removed classes and methods.
- Introduced default theme and mode constants for report styling. - Updated SVG elements to use currentColor for better theme adaptability. - Modified HTML generation to include theme and mode attributes. - Enhanced sidebar and main content styling with W3.CSS classes. - Updated metric card rendering to reflect theme changes. - Adjusted tests to validate new class names and theme integration.
… and improved instructions
…les and loading overlay
…les and loading overlay
…lay, sortable tables, and code refactoring
…ading overlay, sortable tables, and code refactoring
…omponent configurations
…rk/action relationships
Functional Improvements: - Added "View Config" quick-links to each benchmark header for direct navigation to the Benchmark Configuration section. - Implemented hierarchical expandable sections for latency percentile distribution, now including dedicated LLQ and HDR tables for every individual action. - Redesigned chart layout to move legends to the bottom-center, maximizing the horizontal width available for performance data visualization. - Optimized zoom panel positioning and enabled legend scrolling to prevent UI overlap in reports with many actions. - Updated time-series charts to use iteration timestamps on the X-axis and enabled measurement point symbols (circles). - Improved information density by reducing table font size, cell padding, and preventing numeric value wrapping. - Refined chart precision to stop at 99.9999% and replaced "Max" labels with "100%". - Reordered benchmark sections to show the iteration data table before the charts. Architectural & Technical Changes: - Refactored percentile table functions to be fully data-driven and reusable for both aggregate and per-action reporting. - Standardized X-axis data handling to support heterogeneous types (timestamps and numeric metrics) via manual JSON array construction. - Updated component configurations to use unique anchor IDs for deep-linking support. - Replaced "Standard Deviation RT" with "Std Dev RT" across all table headers for conciseness.
… sidebar link styling
…n and table functionality
📝 WalkthroughWalkthroughThis PR introduces the ChangesTulip Report Generator Implementation
Sequence Diagram(s)sequenceDiagram
participant Page as Browser Page
participant DOM as DOM/HTML
participant ECharts as ECharts Charts
participant LocalStorage as localStorage
Page->>DOM: DOMContentLoaded event
DOM->>LocalStorage: Load saved theme
DOM->>ECharts: probeColors() from CSS vars
DOM->>ECharts: updateEchartsTheme()
DOM->>DOM: updateActiveLink() for sidebar
Note over Page: User interactions
alt Table Header Click
Page->>DOM: sortTable(tableId, columnIdx)
DOM->>DOM: Reorder tbody rows by group
DOM->>DOM: Update sort icons
end
alt Theme Toggle Click
Page->>DOM: toggleTheme()
DOM->>LocalStorage: Persist theme choice
DOM->>ECharts: updateEchartsTheme()
ECharts->>ECharts: Resize and refresh
end
alt Chart Fullscreen
Page->>DOM: toggleFullscreen(chartId)
ECharts->>ECharts: resize()
end
alt Download Table Data
Page->>DOM: downloadTableAsCSV/JSON(tableId)
DOM->>DOM: tableToArray(tableId)
DOM->>DOM: arrayToCsv() or JSON.stringify()
DOM->>Page: Trigger browser download
end
sequenceDiagram
participant CLI as TulipReport.main()
participant Generator as TulipReportGenerator
participant DataValidator as ReportDataValidator
participant HTMLBuilder as HTML DSL
participant Adoc as ReportAdoc
CLI->>CLI: validateArgs()
CLI->>Generator: createReport(jsonPath, htmlPath)
Generator->>Generator: readJsonFile(jsonPath)
Generator->>Generator: Json.decodeFromString(ReportData)
Generator->>DataValidator: validate(reportData)
DataValidator->>Generator: List<String> warnings
alt Warnings found
Generator->>HTMLBuilder: renderWarningBanner()
end
Generator->>HTMLBuilder: generateHtml(reportData)
HTMLBuilder->>HTMLBuilder: renderSidebar()
HTMLBuilder->>HTMLBuilder: renderHeader()
HTMLBuilder->>HTMLBuilder: renderOverview()
HTMLBuilder->>HTMLBuilder: renderBenchmarks()
HTMLBuilder->>HTMLBuilder: renderConfig()
HTMLBuilder->>HTMLBuilder: renderFooter()
Generator->>Generator: writeHtml(outputPath)
Generator->>Adoc: generateAsciiDoc(reportData)
Generator->>Generator: writeAdoc(outputPath + "_c.adoc")
Generator->>Adoc: convertAdocToHtml(adocPath)
Adoc->>Adoc: configureAsciidoctor()
Adoc->>Adoc: convertFile() to HTML
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/TulipReportGenerator.kt (1)
168-170: ⚡ Quick winUse stable, sanitized, unique benchmark IDs for anchors.
Current ID derivation only replaces spaces. Names with symbols can break hash navigation/selectors, and duplicate names can collide. Generate slugified IDs and de-duplicate them before rendering nav links/sections.
Proposed direction
- val bmId = bmName.replace(" ", "_") + val bmId = bmName + .lowercase() + .replace(Regex("[^a-z0-9]+"), "_") + .trim('_')Also keep a
MutableSet<String>to append_<n>on collisions.Also applies to: 95-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/TulipReportGenerator.kt` around lines 168 - 170, The current anchor id generation (using bmName.replace and variable bmId) is fragile and can produce invalid or colliding IDs; replace it with a slugify/sanitize step to collapse/strip unsafe characters, normalize to lower-case, and convert spaces to hyphens (e.g., compute baseSlug from bmName), then maintain a MutableSet<String> of used IDs and if baseSlug is already present append a suffix _n (incrementing n) to produce a unique id, assign that unique id to the section (and to any nav/link generation code that currently uses bmId), and apply the same slug+dedupe logic to the earlier occurrence referenced around the other uses (the places corresponding to the earlier bmId creation, e.g., the code at the other location using the same naming approach).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tulip-report/build_and_test.sh`:
- Line 7: The script currently runs cd "$(dirname "$0")/.." without guarding
failure, which could let the later Gradle call run in the wrong directory;
update the script so the cd command fails fast (exit non‑zero) if it cannot
change directories before any Gradle invocation (e.g. the ./gradlew or gradle
call later in the script). Implement this by checking the cd result and exiting
on failure (or enable strict failure handling) immediately after the cd
"$(dirname "$0")/.." command so subsequent commands do not run in the wrong
path.
In `@tulip-report/report.sh`:
- Line 31: The cd invocation cd "$(dirname "$0")/.." must not silently fail;
modify the script so that after running that exact cd command the script tests
its exit status and aborts with a non-zero exit (and a brief error message) if
the cd fails — implement this by appending a fail-fast check (for example using
|| exit 1 or an if-check that echoes a failure and exits) to the cd "$(dirname
"$0")/.." line so Gradle never runs in the wrong directory.
In `@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportAdoc.kt`:
- Around line 54-81: The call to asciidoctor.shutdown() is skipped if
asciidoctor.convertFile(...) throws; wrap the conversion in a try/finally so
shutdown() always runs: after creating Asciidoctor via
Asciidoctor.Factory.create() and after the optional
asciidoctor.requireLibrary(...) block, call asciidoctor.convertFile(...) inside
a try block and invoke asciidoctor.shutdown() in the finally block to guarantee
cleanup even on exceptions.
- Around line 60-63: The Asciidoctor conversion is using insecure sandbox
settings (SafeMode.UNSAFE + allow-uri-read=true); update the configuration in
ReportAdoc so it uses a safer SafeMode (replace SafeMode.UNSAFE with
SafeMode.SAFE for semi-trusted content or SafeMode.SECURE for untrusted input)
and remove or set .attribute("allow-uri-read", false) unless you validate URIs
beforehand; keep the stylesheet attribute (cssUrl) as-is but ensure any needed
external URI retrieval is explicitly validated before re-enabling
allow-uri-read.
In
`@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportChartUtils.kt`:
- Around line 552-573: The code in SCRIPT.renderChartScript injects config.id,
config.title and config.unit directly into single-quoted JS literals which
breaks on embedded quotes and can enable injection; fix by producing safe JS
string literals (e.g. use Json.encodeToString(...) or an equivalent escaping
routine for each of config.id, config.title and config.unit) and insert those
encoded values into the unsafe block (remove the manual surrounding single
quotes since Json.encodeToString already yields a quoted JS/JSON string), so
create${config.type}Chart('${config.id}', ...) becomes
create${config.type}Chart($safeId, $labelsJson, $dataJson, $safeTitle,
$safeUnit) where safeId/safeTitle/safeUnit are the JSON-encoded/escaped forms.
In
`@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportComponents.kt`:
- Around line 279-289: The rendering currently only shows the "No options
found." row when java.runtimeOptions is null, so an empty non-null list renders
nothing; update the block that references java.runtimeOptions?.forEach to
explicitly check for null-or-empty (e.g., use isNullOrEmpty or a null check plus
isEmpty) and render the fallback tr { td { +"No options found." } } when the
list is null or empty, otherwise iterate and render each opt; target the
java.runtimeOptions handling in ReportComponents.kt (the
java.runtimeOptions?.forEach section) to implement this conditional.
- Around line 47-64: The buttons currently interpolate titleSlug and tableId
directly into single-quoted inline onclick handlers (in the button blocks that
call toggleFullscreen, downloadTableAsCSV, downloadTableAsJSON), which allows
quotes to break the JS; fix by outputting safe values instead of raw
interpolation — either (preferable) store tableId/titleSlug in data-* attributes
(e.g. data-table-id/data-title) on the same button elements and change the
onclick to call the JS handler that reads and decodes those dataset values, or
(if keeping inline calls) escape/encode the values server-side (e.g. replace
single-quote and backslash or use URL-encoding like encodeURIComponent on
titleSlug/tableId before interpolating) so toggleFullscreen, downloadTableAsCSV
and downloadTableAsJSON receive safe strings. Ensure changes are applied to the
three button() blocks around titleSlug/tableId and update any client-side code
that reads the new data-* attributes accordingly.
In
`@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportDataValidator.kt`:
- Around line 24-25: The validator currently only treats result.bmName == null
as missing, allowing empty or whitespace-only names to pass; update the check in
ReportDataValidator (where result.bmName is validated, e.g., inside the loop
that uses index and msg) to use Kotlin's isNullOrBlank() (or trim().isEmpty())
so blank names are treated as missing, and adjust the error message if needed to
reflect blank/missing names.
In
`@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportStatsUtils.kt`:
- Around line 83-86: Replace the broad catch of Throwable in the histogram
decode error handler with catch(Exception) to avoid swallowing fatal JVM errors;
update the logger call in the same catch block (the line currently calling
logger.error("Unexpected error decoding histogram: ${e.message}")) to include
the exception object so the stacktrace and full diagnostics are logged (e.g.,
pass e as the throwable argument to logger.error). This change should be applied
to the catch block in ReportStatsUtils (the histogram decoding error handler)
and keep the comment/fallback behavior intact.
In `@tulip-report/src/main/resources/io/github/tulipltt/tulip/report/scripts.js`:
- Around line 432-434: chartDom.closest('article') can return null causing
card.classList.toggle('fullscreen') to throw; guard the result and only toggle
when non-null (or provide a fallback behavior). Update the block using
chartDom.closest('article') (the card variable) so you check card !==
null/undefined before calling card.classList.toggle('fullscreen'), and keep the
subsequent retrieval of the chart via charts.get(chartId) unchanged.
- Around line 288-292: The fallback rendering is inserting untrusted chartTitle
into element.innerHTML, creating an XSS sink; update the error fallback in
scripts.js (the block that sets element.innerHTML for the "Chart Failed to Load"
UI) to avoid string interpolation into innerHTML — instead build the nodes via
createElement/appendChild or set element.textContent for the title portion (or
explicitly HTML-escape chartTitle) so the chartTitle is rendered as plain text
rather than raw HTML.
In
`@tulip-report/src/test/kotlin/io/github/tulipltt/tulip/report/TulipReportGeneratorTest.kt`:
- Around line 11-16: The test TulipReportGeneratorTest currently silently
returns when the fixture file (jsonFile/actualFile) is missing; change it to
load the fixture as a mandatory classpath resource (e.g. use
javaClass.getResource or getResourceAsStream for "/reports/report_output.json")
and fail fast if not found by throwing an assertion/exception (or using Kotlin
test assertions) instead of returning, so the test always fails in CI when the
fixture is absent.
---
Nitpick comments:
In
`@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/TulipReportGenerator.kt`:
- Around line 168-170: The current anchor id generation (using bmName.replace
and variable bmId) is fragile and can produce invalid or colliding IDs; replace
it with a slugify/sanitize step to collapse/strip unsafe characters, normalize
to lower-case, and convert spaces to hyphens (e.g., compute baseSlug from
bmName), then maintain a MutableSet<String> of used IDs and if baseSlug is
already present append a suffix _n (incrementing n) to produce a unique id,
assign that unique id to the section (and to any nav/link generation code that
currently uses bmId), and apply the same slug+dedupe logic to the earlier
occurrence referenced around the other uses (the places corresponding to the
earlier bmId creation, e.g., the code at the other location using the same
naming approach).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e507819-d404-4e00-9885-eedc77efc747
⛔ Files ignored due to path filters (8)
tulip-report/src/main/resources/io/github/tulipltt/tulip/report/echarts.min.jsis excluded by!**/*.min.jstulip-report/src/main/resources/io/github/tulipltt/tulip/report/icons/activity.svgis excluded by!**/*.svgtulip-report/src/main/resources/io/github/tulipltt/tulip/report/icons/dashboard.svgis excluded by!**/*.svgtulip-report/src/main/resources/io/github/tulipltt/tulip/report/icons/info.svgis excluded by!**/*.svgtulip-report/src/main/resources/io/github/tulipltt/tulip/report/icons/maximize.svgis excluded by!**/*.svgtulip-report/src/main/resources/io/github/tulipltt/tulip/report/icons/settings.svgis excluded by!**/*.svgtulip-report/src/main/resources/io/github/tulipltt/tulip/report/icons/theme.svgis excluded by!**/*.svgtulip-report/src/main/resources/io/github/tulipltt/tulip/report/tulip_logo.svgis excluded by!**/*.svg
📒 Files selected for processing (31)
.gitignoresettings.gradle.ktstulip-report/README.adoctulip-report/build.gradle.ktstulip-report/build_and_test.shtulip-report/report.shtulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ChartConfig.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportAdoc.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportChartUtils.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportComponents.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportConstants.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportData.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportDataValidator.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportIcons.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportModels.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportScripts.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportStatsUtils.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportStyles.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportTables.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/StatsCardConfig.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/TulipReport.kttulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/TulipReportGenerator.kttulip-report/src/main/resources/io/github/tulipltt/tulip/report/pico.min.csstulip-report/src/main/resources/io/github/tulipltt/tulip/report/scripts.jstulip-report/src/main/resources/io/github/tulipltt/tulip/report/styles.csstulip-report/src/test/kotlin/io/github/tulipltt/tulip/report/ReportChartUtilsTest.kttulip-report/src/test/kotlin/io/github/tulipltt/tulip/report/ReportComponentsTest.kttulip-report/src/test/kotlin/io/github/tulipltt/tulip/report/ReportDataValidatorTest.kttulip-report/src/test/kotlin/io/github/tulipltt/tulip/report/ReportTablesTest.kttulip-report/src/test/kotlin/io/github/tulipltt/tulip/report/TulipReportGeneratorTest.kttulip-runtime/build.gradle.kts
| # | ||
|
|
||
| # Navigate to project root | ||
| cd "$(dirname "$0")/.." |
There was a problem hiding this comment.
Guard cd failure before invoking Gradle.
Line 7 should fail fast if directory change fails; otherwise Line 10 can execute in the wrong path.
Suggested fix
- cd "$(dirname "$0")/.."
+ cd "$(dirname "$0")/.." || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd "$(dirname "$0")/.." | |
| cd "$(dirname "$0")/.." || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 7-7: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tulip-report/build_and_test.sh` at line 7, The script currently runs cd
"$(dirname "$0")/.." without guarding failure, which could let the later Gradle
call run in the wrong directory; update the script so the cd command fails fast
(exit non‑zero) if it cannot change directories before any Gradle invocation
(e.g. the ./gradlew or gradle call later in the script). Implement this by
checking the cd result and exiting on failure (or enable strict failure
handling) immediately after the cd "$(dirname "$0")/.." command so subsequent
commands do not run in the wrong path.
| fi | ||
|
|
||
| # Run the task using gradlew from project root | ||
| cd "$(dirname "$0")/.." |
There was a problem hiding this comment.
Handle cd failure explicitly.
Line 31 should exit when cd fails to avoid running Gradle in an unintended directory.
Suggested fix
- cd "$(dirname "$0")/.."
+ cd "$(dirname "$0")/.." || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cd "$(dirname "$0")/.." | |
| cd "$(dirname "$0")/.." || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 31-31: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tulip-report/report.sh` at line 31, The cd invocation cd "$(dirname "$0")/.."
must not silently fail; modify the script so that after running that exact cd
command the script tests its exit status and aborts with a non-zero exit (and a
brief error message) if the cd fails — implement this by appending a fail-fast
check (for example using || exit 1 or an if-check that echoes a failure and
exits) to the cd "$(dirname "$0")/.." line so Gradle never runs in the wrong
directory.
Comment on lines
+54
to
+81
| val asciidoctor = Asciidoctor.Factory.create() | ||
| val cssUrl = | ||
| "https://raw.githubusercontent.com/tulipltt/Tulip/" + | ||
| "refs/heads/main/docs/css/adoc-foundation.css" | ||
| val attributes = | ||
| Attributes.builder() | ||
| .attribute("linkcss", false) | ||
| .attribute("data-uri", true) | ||
| .attribute("allow-uri-read", true) | ||
| .attribute("stylesheet", cssUrl) | ||
| .build() | ||
|
|
||
| try { | ||
| asciidoctor.requireLibrary("asciidoctor-diagram") | ||
| } catch (ignore: Exception) { | ||
| // Diagram support is optional | ||
| } | ||
|
|
||
| asciidoctor.convertFile( | ||
| File(adocPath), | ||
| Options.builder() | ||
| .toFile(true) | ||
| .attributes(attributes) | ||
| .safe(SafeMode.UNSAFE) | ||
| .build(), | ||
| ) | ||
| asciidoctor.shutdown() | ||
| } |
There was a problem hiding this comment.
Ensure Asciidoctor.shutdown() runs on failure paths.
If convertFile throws, shutdown() is skipped. Wrap conversion in try/finally to avoid leaking resources.
Proposed fix
fun convertAdocToHtml(adocPath: String) {
val asciidoctor = Asciidoctor.Factory.create()
@@
- asciidoctor.convertFile(
- File(adocPath),
- Options.builder()
- .toFile(true)
- .attributes(attributes)
- .safe(SafeMode.UNSAFE)
- .build(),
- )
- asciidoctor.shutdown()
+ try {
+ asciidoctor.convertFile(
+ File(adocPath),
+ Options.builder()
+ .toFile(true)
+ .attributes(attributes)
+ .safe(SafeMode.UNSAFE)
+ .build(),
+ )
+ } finally {
+ asciidoctor.shutdown()
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val asciidoctor = Asciidoctor.Factory.create() | |
| val cssUrl = | |
| "https://raw.githubusercontent.com/tulipltt/Tulip/" + | |
| "refs/heads/main/docs/css/adoc-foundation.css" | |
| val attributes = | |
| Attributes.builder() | |
| .attribute("linkcss", false) | |
| .attribute("data-uri", true) | |
| .attribute("allow-uri-read", true) | |
| .attribute("stylesheet", cssUrl) | |
| .build() | |
| try { | |
| asciidoctor.requireLibrary("asciidoctor-diagram") | |
| } catch (ignore: Exception) { | |
| // Diagram support is optional | |
| } | |
| asciidoctor.convertFile( | |
| File(adocPath), | |
| Options.builder() | |
| .toFile(true) | |
| .attributes(attributes) | |
| .safe(SafeMode.UNSAFE) | |
| .build(), | |
| ) | |
| asciidoctor.shutdown() | |
| } | |
| val asciidoctor = Asciidoctor.Factory.create() | |
| val cssUrl = | |
| "https://raw.githubusercontent.com/tulipltt/Tulip/" + | |
| "refs/heads/main/docs/css/adoc-foundation.css" | |
| val attributes = | |
| Attributes.builder() | |
| .attribute("linkcss", false) | |
| .attribute("data-uri", true) | |
| .attribute("allow-uri-read", true) | |
| .attribute("stylesheet", cssUrl) | |
| .build() | |
| try { | |
| asciidoctor.requireLibrary("asciidoctor-diagram") | |
| } catch (ignore: Exception) { | |
| // Diagram support is optional | |
| } | |
| try { | |
| asciidoctor.convertFile( | |
| File(adocPath), | |
| Options.builder() | |
| .toFile(true) | |
| .attributes(attributes) | |
| .safe(SafeMode.UNSAFE) | |
| .build(), | |
| ) | |
| } finally { | |
| asciidoctor.shutdown() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportAdoc.kt`
around lines 54 - 81, The call to asciidoctor.shutdown() is skipped if
asciidoctor.convertFile(...) throws; wrap the conversion in a try/finally so
shutdown() always runs: after creating Asciidoctor via
Asciidoctor.Factory.create() and after the optional
asciidoctor.requireLibrary(...) block, call asciidoctor.convertFile(...) inside
a try block and invoke asciidoctor.shutdown() in the finally block to guarantee
cleanup even on exceptions.
Comment on lines
+60
to
+63
| .attribute("linkcss", false) | ||
| .attribute("data-uri", true) | ||
| .attribute("allow-uri-read", true) | ||
| .attribute("stylesheet", cssUrl) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For AsciidoctorJ 3.x, what are the security implications of using SafeMode.UNSAFE together with allow-uri-read=true, and what safer settings are recommended for converting untrusted or semi-trusted AsciiDoc?
💡 Result:
If you use AsciidoctorJ 3.x with SafeMode.UNSAFE together with allow-uri-read=true, you effectively turn off Asciidoctor’s safety controls while also allowing documents to retrieve and embed external content via URIs; this increases the risk of SSRF-like behavior (server-side fetching of attacker-controlled URLs) and makes it more likely that malicious markup can reach the generated output. Security implications of SafeMode.UNSAFE + allow-uri-read=true 1) Unsafe disables Asciidoctor’s security features SafeMode.UNSAFE is defined as disabling any security features enforced by Asciidoctor [1][2]. That means the normal guardrails that limit what the document is allowed to do (includes/macros/scripts/URI-related restrictions, etc.) are not applied. 2) allow-uri-read=true permits retrieving content from URIs Asciidoctor provides a feature “retrieving content from URIs”; SafeMode.SECURE explicitly disables “data can not be retrieved from URIs” [1][2]. By contrast, when you set allow-uri-read=true, you’re explicitly enabling the URI-reading behavior that the more restrictive modes block. Putting (1) and (2) together, you have: (a) no Asciidoctor-enforced security limits (UNSAFE) [1][2], and (b) the ability for the document to read/ingest attacker-influenced remote resources (allow-uri-read=true) [2]. This is a high-risk combination for untrusted or semi-trusted AsciiDoc. 3) Safer modes exist and are recommended for untrusted/semi-trusted conversion The official AsciidoctorJ safe-mode docs recommend setting SAFE when converting AsciiDoc with AsciidoctorJ to “have almost all Asciidoctor features” such as icons, include directive, or retrieving content from URIs enabled [1][2]. The same docs also describe SECURE as the default safe mode for the API, emphasizing that it disallows reading files from the file system and including their contents [1][2]. What’s safer for untrusted or semi-trusted AsciiDoc Recommended baseline: SafeMode.SAFE for “semi-trusted” (still higher risk than SECURE) - Use SafeMode.SAFE in AsciidoctorJ for untrusted/semi-trusted content when you need most features, because it provides the best tradeoff while still applying security constraints [1][2]. - Official guidance explicitly says: “We recommend you to set SAFE safe mode when converting AsciiDoc documents using AsciidoctorJ” [1][2]. Most restrictive: SafeMode.SECURE for strongly untrusted input - The docs state that when AsciidoctorJ is used as an API, it uses SECURE by default and that SECURE is “the most restrictive” mode, disallowing reading files from the file system and including their contents [1][2]. Why you should still treat “untrusted” AsciiDoc as potentially dangerous even in safe modes - The broader Asciidoctor documentation discusses that allowing user-supplied AsciiDoc on the web can lead to XSS/SSRF “by design” even in safe mode, and advises extra care if you host a service for untrusted input [3]. - So for truly untrusted input, you generally want defense-in-depth beyond safe-mode (e.g., output sanitization and strict handling of any embedded HTML/URI content). Practical recommendation matrix - Untrusted AsciiDoc (highest risk): prefer SafeMode.SECURE (and default API behavior), plus sanitize the rendered output if you embed it into a larger HTML context [1][2][3]. Avoid SafeMode.UNSAFE. - Semi-trusted AsciiDoc (you can control environment and accept some limitations): prefer SafeMode.SAFE; only enable URI reading (allow-uri-read) if it’s necessary and you can constrain/validate permitted URI schemes/domains at the application level. Example (AsciidoctorJ API) Use SAFE explicitly: Options options = Options.builder.safe(SafeMode.SAFE).build; String outfile = asciidoctor.convertFile(new File("sample.adoc"), options); This matches the official example for setting SAFE in AsciidoctorJ [1]. In short: SafeMode.UNSAFE removes Asciidoctor’s security protections [1][2], and combining that with allow-uri-read=true allows remote content retrieval, which is unsafe for untrusted input. The official safer defaults to consider are SafeMode.SAFE (recommended for most feature needs) or SafeMode.SECURE (default and most restrictive) [1][2].
Citations:
- 1: https://docs.asciidoctor.org/asciidoctorj/latest/safe-mode-and-filesystem-access
- 2: https://docs.asciidoctor.org/asciidoctorj/latest/safe-mode-and-filesystem-access/
- 3: Document security assumptions asciidoctor/asciidoctor.org#820
Harden Asciidoctor conversion sandbox settings.
SafeMode.UNSAFE combined with allow-uri-read=true disables Asciidoctor's security features while explicitly enabling external URI retrieval—creating significant risk of SSRF-like behavior and injection attacks when processing untrusted or semi-trusted AsciiDoc.
For semi-trusted content, use SafeMode.SAFE (official recommendation). For truly untrusted input, use SafeMode.SECURE (default and most restrictive). Disable allow-uri-read unless URI retrieval is strictly necessary and validated at the application level.
This applies to lines 62 and 77.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportAdoc.kt`
around lines 60 - 63, The Asciidoctor conversion is using insecure sandbox
settings (SafeMode.UNSAFE + allow-uri-read=true); update the configuration in
ReportAdoc so it uses a safer SafeMode (replace SafeMode.UNSAFE with
SafeMode.SAFE for semi-trusted content or SafeMode.SECURE for untrusted input)
and remove or set .attribute("allow-uri-read", false) unless you validate URIs
beforehand; keep the stylesheet attribute (cssUrl) as-is but ensure any needed
external URI retrieval is explicitly validated before re-enabling
allow-uri-read.
Comment on lines
+552
to
+573
| private fun SCRIPT.renderChartScript(config: ChartConfig) { | ||
| val labelsJson = Json.encodeToString(config.labels) | ||
|
|
||
| // Manually build JsonArray to handle heterogeneous types (String, Double, null) | ||
| val dataJson = buildJsonArray { | ||
| config.data.forEach { row -> | ||
| add(buildJsonArray { | ||
| row.forEach { cell -> | ||
| when (cell) { | ||
| is String -> add(cell) | ||
| is Number -> add(cell) | ||
| null -> add(JsonNull) | ||
| else -> add(cell.toString()) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| }.toString() | ||
|
|
||
| unsafe { | ||
| +"create${config.type}Chart('${config.id}', $labelsJson, $dataJson, '${config.title}', '${config.unit}');" | ||
| } |
There was a problem hiding this comment.
Escape JS string arguments in chart script emission.
Line 572 injects config.id, config.title, and config.unit directly into JS string literals. A single quote in input breaks rendering and can enable script injection.
Proposed fix
private fun SCRIPT.renderChartScript(config: ChartConfig) {
val labelsJson = Json.encodeToString(config.labels)
+ val idJson = Json.encodeToString(config.id)
+ val titleJson = Json.encodeToString(config.title)
+ val unitJson = Json.encodeToString(config.unit)
+ val chartFactory =
+ when (config.type) {
+ "TimeSeries" -> "createTimeSeriesChart"
+ "Percentile" -> "createPercentileChart"
+ else -> error("Unsupported chart type: ${config.type}")
+ }
// Manually build JsonArray to handle heterogeneous types (String, Double, null)
val dataJson = buildJsonArray {
config.data.forEach { row ->
add(buildJsonArray {
@@
}.toString()
unsafe {
- +"create${config.type}Chart('${config.id}', $labelsJson, $dataJson, '${config.title}', '${config.unit}');"
+ +"$chartFactory($idJson, $labelsJson, $dataJson, $titleJson, $unitJson);"
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private fun SCRIPT.renderChartScript(config: ChartConfig) { | |
| val labelsJson = Json.encodeToString(config.labels) | |
| // Manually build JsonArray to handle heterogeneous types (String, Double, null) | |
| val dataJson = buildJsonArray { | |
| config.data.forEach { row -> | |
| add(buildJsonArray { | |
| row.forEach { cell -> | |
| when (cell) { | |
| is String -> add(cell) | |
| is Number -> add(cell) | |
| null -> add(JsonNull) | |
| else -> add(cell.toString()) | |
| } | |
| } | |
| }) | |
| } | |
| }.toString() | |
| unsafe { | |
| +"create${config.type}Chart('${config.id}', $labelsJson, $dataJson, '${config.title}', '${config.unit}');" | |
| } | |
| private fun SCRIPT.renderChartScript(config: ChartConfig) { | |
| val labelsJson = Json.encodeToString(config.labels) | |
| val idJson = Json.encodeToString(config.id) | |
| val titleJson = Json.encodeToString(config.title) | |
| val unitJson = Json.encodeToString(config.unit) | |
| val chartFactory = | |
| when (config.type) { | |
| "TimeSeries" -> "createTimeSeriesChart" | |
| "Percentile" -> "createPercentileChart" | |
| else -> error("Unsupported chart type: ${config.type}") | |
| } | |
| // Manually build JsonArray to handle heterogeneous types (String, Double, null) | |
| val dataJson = buildJsonArray { | |
| config.data.forEach { row -> | |
| add(buildJsonArray { | |
| row.forEach { cell -> | |
| when (cell) { | |
| is String -> add(cell) | |
| is Number -> add(cell) | |
| null -> add(JsonNull) | |
| else -> add(cell.toString()) | |
| } | |
| } | |
| }) | |
| } | |
| }.toString() | |
| unsafe { | |
| "$chartFactory($idJson, $labelsJson, $dataJson, $titleJson, $unitJson);" | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportChartUtils.kt`
around lines 552 - 573, The code in SCRIPT.renderChartScript injects config.id,
config.title and config.unit directly into single-quoted JS literals which
breaks on embedded quotes and can enable injection; fix by producing safe JS
string literals (e.g. use Json.encodeToString(...) or an equivalent escaping
routine for each of config.id, config.title and config.unit) and insert those
encoded values into the unsafe block (remove the manual surrounding single
quotes since Json.encodeToString already yields a quoted JS/JSON string), so
create${config.type}Chart('${config.id}', ...) becomes
create${config.type}Chart($safeId, $labelsJson, $dataJson, $safeTitle,
$safeUnit) where safeId/safeTitle/safeUnit are the JSON-encoded/escaped forms.
Comment on lines
+24
to
+25
| if (result.bmName == null) { | ||
| val msg = "Benchmark result at index $index is missing a name." |
There was a problem hiding this comment.
Treat blank benchmark names as missing.
Line 24 only checks null. Empty/blank names currently pass validation and still break naming in output.
Suggested fix
- if (result.bmName == null) {
+ if (result.bmName.isNullOrBlank()) {
val msg = "Benchmark result at index $index is missing a name."
logger.warn(msg)
warnings.add(msg)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (result.bmName == null) { | |
| val msg = "Benchmark result at index $index is missing a name." | |
| if (result.bmName.isNullOrBlank()) { | |
| val msg = "Benchmark result at index $index is missing a name." | |
| logger.warn(msg) | |
| warnings.add(msg) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportDataValidator.kt`
around lines 24 - 25, The validator currently only treats result.bmName == null
as missing, allowing empty or whitespace-only names to pass; update the check in
ReportDataValidator (where result.bmName is validated, e.g., inside the loop
that uses index and msg) to use Kotlin's isNullOrBlank() (or trim().isEmpty())
so blank names are treated as missing, and adjust the error message if needed to
reflect blank/missing names.
Comment on lines
+83
to
+86
| } catch (@Suppress("TooGenericExceptionCaught") e: Throwable) { | ||
| // Fallback for other decoding issues that aren't IllegalArgumentException | ||
| logger.error("Unexpected error decoding histogram: ${e.message}") | ||
| null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no other generic Throwable catches were introduced in this module.
rg -n -C2 'catch\s*\([^)]*Throwable' tulip-report/src/main/kotlinRepository: tulipltt/Tulip
Length of output: 40
🏁 Script executed:
# First, locate and read the specific file
fd -t f "ReportStatsUtils.kt" tulip-report/src/main/kotlinRepository: tulipltt/Tulip
Length of output: 139
🏁 Script executed:
# Improved search for Throwable catches, accounting for annotations and whitespace
rg -n 'catch\s*\(\s*(@[^)]*\s+)*.*Throwable' tulip-report/src/main/kotlin -A 2Repository: tulipltt/Tulip
Length of output: 535
🏁 Script executed:
# Alternative: search more broadly for Throwable in catch blocks
rg -n 'Throwable' tulip-report/src/main/kotlin -B 2 -A 2Repository: tulipltt/Tulip
Length of output: 808
🏁 Script executed:
# Search for Throwable catches throughout tulip-report module
rg -n 'catch.*Throwable' tulip-report/ -A 1Repository: tulipltt/Tulip
Length of output: 377
🏁 Script executed:
# Read the file to see full method context and verify lines around 83-86
cat -n tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportStatsUtils.kt | sed -n '70,90p'Repository: tulipltt/Tulip
Length of output: 1030
Change Throwable to Exception in histogram decode error handler.
Catching Throwable at line 83 can hide fatal JVM errors (OutOfMemoryError, StackOverflowError, etc.), masking critical failures. This catch is intended only for decoding exceptions, which are Exception subclasses. Change to Exception and add the exception to logging for better diagnostics.
Suggested fix
- } catch (`@Suppress`("TooGenericExceptionCaught") e: Throwable) {
+ } catch (e: Exception) {
// Fallback for other decoding issues that aren't IllegalArgumentException
- logger.error("Unexpected error decoding histogram: ${e.message}")
+ logger.error("Unexpected error decoding histogram: ${e.message}", e)
null
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (@Suppress("TooGenericExceptionCaught") e: Throwable) { | |
| // Fallback for other decoding issues that aren't IllegalArgumentException | |
| logger.error("Unexpected error decoding histogram: ${e.message}") | |
| null | |
| } catch (e: Exception) { | |
| // Fallback for other decoding issues that aren't IllegalArgumentException | |
| logger.error("Unexpected error decoding histogram: ${e.message}", e) | |
| null |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tulip-report/src/main/kotlin/io/github/tulipltt/tulip/report/ReportStatsUtils.kt`
around lines 83 - 86, Replace the broad catch of Throwable in the histogram
decode error handler with catch(Exception) to avoid swallowing fatal JVM errors;
update the logger call in the same catch block (the line currently calling
logger.error("Unexpected error decoding histogram: ${e.message}")) to include
the exception object so the stacktrace and full diagnostics are logged (e.g.,
pass e as the throwable argument to logger.error). This change should be applied
to the catch block in ReportStatsUtils (the histogram decoding error handler)
and keep the comment/fallback behavior intact.
Comment on lines
+288
to
+292
| element.innerHTML = ` | ||
| <div style="padding: 2rem; text-align: center; color: var(--pico-color);"> | ||
| <p><strong style="color: var(--pico-danger-text);">⚠ Chart Failed to Load</strong></p> | ||
| <p>Could not render chart: ${chartTitle}</p> | ||
| <p><small>Please check the console for details.</small></p> |
There was a problem hiding this comment.
Avoid HTML injection in fallback rendering.
chartTitle is interpolated into innerHTML (Line 291). If title text is untrusted, this is an XSS sink when fallback UI is rendered.
Proposed fix
- element.innerHTML = `
- <div style="padding: 2rem; text-align: center; color: var(--pico-color);">
- <p><strong style="color: var(--pico-danger-text);">⚠ Chart Failed to Load</strong></p>
- <p>Could not render chart: ${chartTitle}</p>
- <p><small>Please check the console for details.</small></p>
- </div>
- `;
+ const wrapper = document.createElement('div');
+ wrapper.style.padding = '2rem';
+ wrapper.style.textAlign = 'center';
+ wrapper.style.color = 'var(--pico-color)';
+
+ const titleEl = document.createElement('p');
+ const strong = document.createElement('strong');
+ strong.style.color = 'var(--pico-danger-text)';
+ strong.textContent = '⚠ Chart Failed to Load';
+ titleEl.appendChild(strong);
+
+ const msg = document.createElement('p');
+ msg.textContent = `Could not render chart: ${chartTitle}`;
+
+ const hint = document.createElement('p');
+ const small = document.createElement('small');
+ small.textContent = 'Please check the console for details.';
+ hint.appendChild(small);
+
+ wrapper.append(titleEl, msg, hint);
+ element.replaceChildren(wrapper);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| element.innerHTML = ` | |
| <div style="padding: 2rem; text-align: center; color: var(--pico-color);"> | |
| <p><strong style="color: var(--pico-danger-text);">⚠ Chart Failed to Load</strong></p> | |
| <p>Could not render chart: ${chartTitle}</p> | |
| <p><small>Please check the console for details.</small></p> | |
| const wrapper = document.createElement('div'); | |
| wrapper.style.padding = '2rem'; | |
| wrapper.style.textAlign = 'center'; | |
| wrapper.style.color = 'var(--pico-color)'; | |
| const titleEl = document.createElement('p'); | |
| const strong = document.createElement('strong'); | |
| strong.style.color = 'var(--pico-danger-text)'; | |
| strong.textContent = '⚠ Chart Failed to Load'; | |
| titleEl.appendChild(strong); | |
| const msg = document.createElement('p'); | |
| msg.textContent = `Could not render chart: ${chartTitle}`; | |
| const hint = document.createElement('p'); | |
| const small = document.createElement('small'); | |
| small.textContent = 'Please check the console for details.'; | |
| hint.appendChild(small); | |
| wrapper.append(titleEl, msg, hint); | |
| element.replaceChildren(wrapper); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tulip-report/src/main/resources/io/github/tulipltt/tulip/report/scripts.js`
around lines 288 - 292, The fallback rendering is inserting untrusted chartTitle
into element.innerHTML, creating an XSS sink; update the error fallback in
scripts.js (the block that sets element.innerHTML for the "Chart Failed to Load"
UI) to avoid string interpolation into innerHTML — instead build the nodes via
createElement/appendChild or set element.textContent for the title portion (or
explicitly HTML-escape chartTitle) so the chartTitle is rendered as plain text
rather than raw HTML.
Comment on lines
+432
to
+434
| const card = chartDom.closest('article'); | ||
| card.classList.toggle('fullscreen'); | ||
| const chart = charts.get(chartId); |
There was a problem hiding this comment.
Guard closest('article') before toggling classes.
If no ancestor <article> exists, Line 433 will throw on card.classList. Add a null check fallback.
Proposed fix
const card = chartDom.closest('article');
- card.classList.toggle('fullscreen');
+ if (!card) return;
+ card.classList.toggle('fullscreen');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const card = chartDom.closest('article'); | |
| card.classList.toggle('fullscreen'); | |
| const chart = charts.get(chartId); | |
| const card = chartDom.closest('article'); | |
| if (!card) return; | |
| card.classList.toggle('fullscreen'); | |
| const chart = charts.get(chartId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tulip-report/src/main/resources/io/github/tulipltt/tulip/report/scripts.js`
around lines 432 - 434, chartDom.closest('article') can return null causing
card.classList.toggle('fullscreen') to throw; guard the result and only toggle
when non-null (or provide a fallback behavior). Update the block using
chartDom.closest('article') (the card variable) so you check card !==
null/undefined before calling card.classList.toggle('fullscreen'), and keep the
subsequent retrieval of the chart via charts.get(chartId) unchanged.
Comment on lines
+11
to
+16
| val jsonFile = File("../reports/report_output.json") | ||
| val actualFile = if (jsonFile.exists()) jsonFile else File("reports/report_output.json") | ||
|
|
||
| if (!actualFile.exists()) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Don’t silently skip this test when the fixture is missing.
This early return makes the test pass without validating anything, which can mask regressions in CI. Make the fixture mandatory (classpath resource) and fail fast if absent.
💡 Suggested fix
- val jsonFile = File("../reports/report_output.json")
- val actualFile = if (jsonFile.exists()) jsonFile else File("reports/report_output.json")
-
- if (!actualFile.exists()) {
- return
- }
+ val fixture = requireNotNull(javaClass.getResource("/report_output.json")) {
+ "Missing test fixture: /report_output.json (place it under src/test/resources)"
+ }
+ val actualFile = File(fixture.toURI())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val jsonFile = File("../reports/report_output.json") | |
| val actualFile = if (jsonFile.exists()) jsonFile else File("reports/report_output.json") | |
| if (!actualFile.exists()) { | |
| return | |
| } | |
| val fixture = requireNotNull(javaClass.getResource("/report_output.json")) { | |
| "Missing test fixture: /report_output.json (place it under src/test/resources)" | |
| } | |
| val actualFile = File(fixture.toURI()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tulip-report/src/test/kotlin/io/github/tulipltt/tulip/report/TulipReportGeneratorTest.kt`
around lines 11 - 16, The test TulipReportGeneratorTest currently silently
returns when the fixture file (jsonFile/actualFile) is missing; change it to
load the fixture as a mandatory classpath resource (e.g. use
javaClass.getResource or getResourceAsStream for "/reports/report_output.json")
and fail fast if not found by throwing an assertion/exception (or using Kotlin
test assertions) instead of returning, so the test always fails in CI when the
fixture is absent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces the tulip-report module, which provides functionality for generating and validating benchmark reports. The new module includes utilities for report data generation, chart
handling, and data validation, ensuring robust reporting capabilities within the Tulip project.
Changes
Quality Assurance
Static Analysis (Detekt)
Test Coverage (JaCoCo)
Verification
Summary by CodeRabbit
Release Notes
New Features
Chores