feat: add OctoFHIR support to benchmark reports and UI components#23
feat: add OctoFHIR support to benchmark reports and UI components#23octoshikari wants to merge 3 commits into
Conversation
- Updated pnpm workspace to ignore built dependencies for sharp and unrs-resolver. - Added OctoFHIR image to the public images directory. - Enhanced BarChart component to include OctoFHIR in chart configuration. - Modified ReportSummary component to display OctoFHIR stats and images. - Updated Suite component to calculate and display OctoFHIR metrics. - Adjusted benchmark data conversion and parsing to include OctoFHIR. - Ensured backward compatibility for reports without OctoFHIR data. - Expanded types to include OctoFHIR in benchmark data structures.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e795366. Configure here.
| medplum: { label: "Medplum", }, | ||
| hapi: { label: "Hapi", } | ||
| hapi: { label: "Hapi", }, | ||
| octofhir: { label: "OctoFHIR", } |
There was a problem hiding this comment.
Missing CSS color variable for OctoFHIR chart bars
High Severity
Adding octofhir to chartConfig in BarChart.tsx introduced a new server without a corresponding --color-octofhir CSS variable in globals.css. Because chart bars use fill={var(--color-${key})} and no inline style is generated for octofhir, its bars render invisible/transparent.
Reviewed by Cursor Bugbot for commit e795366. Configure here.
| medplum: testCase.data.reduce((sum: number, dp: any) => sum + dp.medplum, 0) / testCase.data.length, | ||
| hapi: testCase.data.reduce((sum: number, dp: any) => sum + dp.hapi, 0) / testCase.data.length | ||
| hapi: testCase.data.reduce((sum: number, dp: any) => sum + dp.hapi, 0) / testCase.data.length, | ||
| octofhir: testCase.data.reduce((sum: number, dp: any) => sum + dp.octofhir, 0) / testCase.data.length |
There was a problem hiding this comment.
OctoFHIR zero values win latency comparisons in old reports
High Severity
The parser defaults missing OctoFHIR data in older reports to 0. In calculateServerStats, this causes OctoFHIR to incorrectly win all 'lower is better' (e.g., latency in ms) benchmarks, as 0 is always the lowest value. This corrupts the Performance Summary's win counts, falsely indicating OctoFHIR as the best performer in tests it wasn't part of.
Reviewed by Cursor Bugbot for commit e795366. Configure here.
| hapi-checker: | ||
| condition: service_healthy | ||
| octofhir: | ||
| condition: service_healthy |
There was a problem hiding this comment.
k6 hard dependency on octofhir blocks all benchmarks
High Severity
The k6 service now unconditionally depends on octofhir with condition: service_healthy, but octofhir uses pull_policy: never and is not included in the bootstrap_services() pull list (which only pulls aidbox hapi medplum). This means docker compose run k6 and docker compose up -d --wait will fail for any user who doesn't have the octofhir image pre-loaded locally — blocking ALL benchmark runs, even those targeting only aidbox, hapi, or medplum.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e795366. Configure here.
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
1 participant
Note
Medium Risk
Moderate risk because it expands the benchmark data schema and parsing/UI logic to include a new server, which could break report rendering or aggregation if any paths assume the previous 3-server shape. Infra changes add a new container/database/metrics target that could affect local perf-test environments but not production code paths.
Overview
Adds OctoFHIR as a first-class benchmark target across the perf stack.
The perf environment now spins up an
octofhircontainer (with Postgres DB init, Prometheus scrape config, andk6dependency), andrunner.shcan run tests against it. Benchmark report generation/parsing/types and UI visualizations (ReportBarChart,Suite,ReportSummary) are extended to includeoctofhir, with backwards-compatible parsing for older reports missing that field.Also loosens the k6 CRUD delete assertion to accept
204in addition to200, and updatespnpm-workspace.yamlto ignore build-time deps (sharp,unrs-resolver).Reviewed by Cursor Bugbot for commit e795366. Bugbot is set up for automated code reviews on this repo. Configure here.