Instrument PDF generation and make fields optional#513
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds structured PDF performance instrumentation across controllers, caching, document generation, and image processing. It also makes selected assessment fields optional for completion and updates unit validation, form locals, display formatting, and associated tests. ChangesPDF performance instrumentation
Assessment completion and unit forms
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UnitsController
participant PdfCacheService
participant PdfGeneratorService
participant PdfPerformance
Client->>UnitsController: request unit PDF
UnitsController->>PdfPerformance: measure record_load and total
UnitsController->>PdfCacheService: fetch_or_generate_unit_pdf
PdfCacheService->>PdfPerformance: measure cache lookup
PdfCacheService->>PdfGeneratorService: generate unit report
PdfGeneratorService->>PdfPerformance: measure report sections and rendering
PdfPerformance-->>Client: structured PDF timing logs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@app/models/unit.rb`:
- Line 70: Resolve the mismatch between the model validation and the unique
`(manufacturer, serial)` index around the Unit validations: normalize blank
manufacturer values to NULL before persistence, or revise the index to enforce
the intended per-user serial uniqueness. Add a regression spec covering
identical serials submitted by different users with blank manufacturer, ensuring
the expected validation behavior rather than RecordNotUnique.
In `@app/services/pdf_generator_service.rb`:
- Around line 95-107: Update generate_unit_details_with_inspection to pass the
already loaded last_inspection into HeaderGenerator and TableBuilder, and modify
those helpers to use the provided inspection instead of calling
unit.last_inspection. Preserve the existing behavior when no inspection is
available.
In `@app/services/pdf_generator_service/image_processor.rb`:
- Around line 56-66: Extract the duplicated
PdfPerformance.measure(:image_transcode, **context) block into a shared private
helper, such as transcode_to_png(image, context), and update both
process_image_with_orientation and render_processed_image to call it. Preserve
the existing PNG output and performance context behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c779df99-0cf1-4824-88f1-cfead253ec58
📒 Files selected for processing (30)
app/controllers/inspections_controller.rbapp/controllers/units_controller.rbapp/models/assessment_schema.rbapp/models/concerns/assessment_completion.rbapp/models/inspection.rbapp/models/unit.rbapp/services/pdf_cache_service.rbapp/services/pdf_generator_service.rbapp/services/pdf_generator_service/image_processor.rbapp/services/pdf_generator_service/photos_renderer.rbapp/services/pdf_performance.rbapp/views/units/_form.html.erbapp/views/units/edit.html.erbapp/views/units/new.html.erbapp/views/units/new_from_inspection.html.erbconfig/forms/structure_assessment.ymlconfig/forms/user_height_assessment.ymlconfig/locales/units.en.ymlspec/features/assessment_forms/structure_assessment_trough_fields_spec.rbspec/features/inspections/inspection_incomplete_fields_spec.rbspec/features/pdfs/pdf_integration_spec.rbspec/features/units/units_form_spec.rbspec/models/assessment_schema_spec.rbspec/models/concerns/assessment_completion_spec.rbspec/models/inspection_spec.rbspec/models/unit_spec.rbspec/requests/inspections/inspections_spec.rbspec/services/pdf_generator_service/image_processor_spec.rbspec/services/pdf_generator_service/photos_renderer_spec.rbspec/services/pdf_performance_spec.rb
💤 Files with no reviewable changes (2)
- app/services/pdf_generator_service/photos_renderer.rb
- config/locales/units.en.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@spec/features/details_links_spec.rb`:
- Line 34: Move the containing_wall_height mutation in the details links setup
before the page navigation, or reload the page after updating it, so the
rendered incomplete-fields links reflect the nil value. Keep the existing
inspection setup and assertions unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c5510ff-3f9e-46e9-89e3-26e26b4f49d6
📒 Files selected for processing (5)
spec/features/details_links_spec.rbspec/features/inspections/turbo_incomplete_fields_spec.rbspec/lib/form_yaml_database_schema_spec.rbspec/support/test_translations.en.ymlspec/support/test_translations.rb
|
Addressed all four actionable review threads in
Verification: 119 focused examples and the full 2,927-example suite pass. StandardRB, Sorbet changed-code checks, and whitespace checks pass. No change was made for the automated docstring-coverage warning because this repository does not use a docstring requirement and its comments policy discourages boilerplate comments; the changed typed code passes the Sorbet signature check. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/services/pdf_generator_service.rb (1)
95-107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOrder
completed_inspectionsbycomplete_date
inspection_datecan differ fromcomplete_date, so this can pick a different “last” inspection thanUnit#last_inspectionand make the PDF header/details inconsistent with the rest of the app. Usecomplete_date: :deschere to keep the same inspection throughout.🤖 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 `@app/services/pdf_generator_service.rb` around lines 95 - 107, Update the inspection history query in the PdfPerformance.measure block to order completed inspections by complete_date descending instead of inspection_date, ensuring completed_inspections.first matches Unit#last_inspection and keeps the PDF data consistent.
🤖 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.
Outside diff comments:
In `@app/services/pdf_generator_service.rb`:
- Around line 95-107: Update the inspection history query in the
PdfPerformance.measure block to order completed inspections by complete_date
descending instead of inspection_date, ensuring completed_inspections.first
matches Unit#last_inspection and keeps the PDF data consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4abd8edd-5f4d-4e66-bc36-83e3d42f7892
📒 Files selected for processing (13)
app/models/unit.rbapp/services/pdf_generator_service.rbapp/services/pdf_generator_service/header_generator.rbapp/services/pdf_generator_service/image_processor.rbapp/services/pdf_generator_service/table_builder.rbdb/migrate/20260715120000_remove_manufacturer_serial_index_from_units.rbdb/schema.rbspec/factories/units.rbspec/features/details_links_spec.rbspec/models/unit_spec.rbspec/seeds/seeds_spec.rbspec/services/pdf_generator_service/table_builder_spec.rbspec/services/pdf_generator_service_spec.rb
💤 Files with no reviewable changes (2)
- spec/factories/units.rb
- app/models/unit.rb
|
Addressed the follow-up outside-diff ordering comment in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@spec/services/pdf_generator_service_spec.rb`:
- Around line 168-186: Extend the “uses the most recently completed inspection
for unit details” example to also verify the inspection passed into
generate_unit_details_with_inspection or TableBuilder, or assert distinctive
rendered details content from passed_inspection. Keep the existing
HeaderGenerator expectation and ensure the assertion confirms passed_inspection
is used for both the header and unit details.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30ecea00-2380-40a1-8ac4-b0a6a10fcbb2
📒 Files selected for processing (2)
app/services/pdf_generator_service.rbspec/services/pdf_generator_service_spec.rb
1 participant
Summary
(manufacturer, serial)unique index while retaining per-user serial uniquenessTesting
Summary by CodeRabbit