[IP-130]: Report Builder — bands + bricks (IP-130, Phases 1-4 + Browsershot driver) - #608
Open
nielsdrost7 wants to merge 15 commits into
Open
[IP-130]: Report Builder — bands + bricks (IP-130, Phases 1-4 + Browsershot driver)#608nielsdrost7 wants to merge 15 commits into
nielsdrost7 wants to merge 15 commits into
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 50 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (89)
📝 WalkthroughWalkthroughAdds a file-backed report template system with Mason bricks, admin/company builders, template cloning and validation, HTML/PDF rendering, PDF downloads, system-template synchronization, and comprehensive tests. It also updates PDF drivers, dependency constraints, styling, translations, and several unrelated formatting/query details. ChangesReport builder and template model
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 13, 2026
nielsdrost7
changed the title
Ran vendor/bin/pint on develop: import ordering, binary-operator spacing, and empty-body brace style. Purely cosmetic, no behavior change.
This was referenced Jul 18, 2026
5 tasks
…ed_at Expense, Payment, Project, and Task all set $timestamps = false and have no created_at column, but the RecentExpensesWidget/RecentPaymentsWidget/ RecentProjectsWidget/RecentTasksWidget dashboard widgets called ->latest() with no argument, which defaults to ordering by created_at regardless of $timestamps. MySQL (CI) raises a hard 1054 Unknown column error; SQLite silently reinterprets the double-quoted "created_at" identifier as a string literal when it can't resolve it, so the bug never surfaced in local sqlite-backed test runs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
resources/lang/en/ip.php-1230-1230 (1)
1230-1230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the duplicate
pdf_templatetranslation key.
pdf_templateis already defined earlier in this file. PHP keeps only the last value for duplicate array keys in the associative array, which can silently mask future translation changes. Reuse the existing key or remove this duplicate.🤖 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 `@resources/lang/en/ip.php` at line 1230, Remove the later duplicate pdf_template entry from the translation array in ip.php, preserving the earlier existing definition and its value.CLAUDE.md-5-13 (1)
5-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign upstream documentation with the Laravel 13 / PHP 8.3+ upgrade.
The documented toolchain matches
composer.json/composer.locknow, but.github/CONTRIBUTING.md,.github/scripts/README.md, and.github/copilot-instructions.mdstill enforce Laravel 11 / PHP 8.1 guidance instead of the current Laravel 13 / PHP 8.3 target, so those project rules need the same update rather than revertingCLAUDE.md.🤖 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 `@CLAUDE.md` around lines 5 - 13, Update the Laravel and PHP version guidance in .github/CONTRIBUTING.md, .github/scripts/README.md, and .github/copilot-instructions.md to match Laravel 13 and PHP 8.3+ (PHP ^8.3), including any related upgrade or tooling instructions. Keep the existing project-specific guidance intact and do not change CLAUDE.md.Source: Coding guidelines
Modules/Core/Tests/Unit/MasonBricksTest.php-231-238 (1)
231-238: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the complete registered brick set.
These tests claim to validate “all bricks” but list only six classes, excluding newly added bricks such as
DetailInvoiceProjectBrick,DetailQuoteProductBrick, andFooterSummaryBrick. Extend the lists—or derive them from the collection—so ID, label, and icon regressions cannot bypass coverage. Based on provided context,ReportBricksCollectionregisters additional detail and footer bricks.Also applies to: 252-259, 273-280
🤖 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 `@Modules/Core/Tests/Unit/MasonBricksTest.php` around lines 231 - 238, The brick lists used by the ID, label, and icon tests in the relevant test methods must cover every class registered by ReportBricksCollection, including DetailInvoiceProjectBrick, DetailQuoteProductBrick, FooterSummaryBrick, and any other registered bricks. Extend the shared expected lists or derive them from the collection so all three assertions validate the complete registered brick set.Modules/Core/Services/ReportDataMapper.php-27-33 (1)
27-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWire
po_numberto a real invoice reference field.
ReportDataMapper::forInvoice()always returns'', soHeaderInvoiceMetaBrick’sshow_po_numbersetting never shows an actual PO/reference value. Mapclient_referenceorwork_orderfrom the invoice instead, or remove the unusedpo_numberkey/config.🤖 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 `@Modules/Core/Services/ReportDataMapper.php` around lines 27 - 33, Update the invoice mapping in ReportDataMapper::forInvoice() so po_number uses the invoice’s real reference field, preferring client_reference or work_order according to the domain model, instead of always returning an empty string. Preserve the existing output key and formatting so HeaderInvoiceMetaBrick can display the configured PO/reference value.Modules/Core/Mason/Bricks/FooterTermsBrick.php-57-66 (1)
57-66: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSanitize
terms_contentbefore output.
index.blade.phprenders$config['terms_content']with escaped output, but the existing default templates use raw description output ({!! ... !!}), and this report HTML can be rendered by the optional Browsershot Chromium driver (IP_PDF_DRIVER=Browsershot) with file access enabled. Either strip/sanitize script/content tags when rendering terms content, or avoid raw HTML when allowing rich content in optional Chromium-generated reports.🤖 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 `@Modules/Core/Mason/Bricks/FooterTermsBrick.php` around lines 57 - 66, Sanitize the rich HTML produced by the terms_content field in FooterTermsBrick before it reaches report rendering, removing script and other executable/content-dangerous tags while preserving supported formatting. Ensure index.blade.php and any default template path do not emit unsanitized terms_content as raw HTML, including when the Browsershot driver is used.Modules/Core/Mason/Bricks/DetailQuoteProjectBrick.php-24-24 (1)
24-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winApply Pint’s union-type formatting consistently. These declarations use spaces around
|, which is not Pint’s standard union-type form.
Modules/Core/Mason/Bricks/DetailQuoteProjectBrick.php#L24-L24: change the return type tostring|Htmlable|null.Modules/Core/Mason/Bricks/DetailTasksBrick.php#L25-L25: change the return type tostring|Htmlable|null.Modules/Core/Mason/Bricks/SpacerBrick.php#L24-L24: change the return type tostring|Htmlable|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 `@Modules/Core/Mason/Bricks/DetailQuoteProjectBrick.php` at line 24, Apply Pint’s union-type formatting to getIcon in Modules/Core/Mason/Bricks/DetailQuoteProjectBrick.php:24-24, Modules/Core/Mason/Bricks/DetailTasksBrick.php:25-25, and Modules/Core/Mason/Bricks/SpacerBrick.php:24-24 by removing spaces around each | in the string|Htmlable|null return type.Source: Coding guidelines
Modules/Core/Tests/Unit/ReportBrickTest.php-42-44 (1)
42-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep brick execution in the Act phase.
Lines 42-44 and 71-73 invoke every brick while asserting. Materialize config/render results under
/* Act */, then assert those values.Also applies to: 71-73
🤖 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 `@Modules/Core/Tests/Unit/ReportBrickTest.php` around lines 42 - 44, Update the affected tests in ReportBrickTest so the Act phase first materializes each brick’s configKeys and render results, then the Assert phase validates those stored values. Keep brick method execution out of the assertions while preserving the existing coverage and failure messages.Source: Coding guidelines
Modules/Core/Console/ReportsSyncSystemCommand.php-21-46 (1)
21-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Sync" only adds/overwrites files, never removes stale ones.
If a shipped template file/folder is removed or renamed in
resources/report-templatesin a future release, the old copy stays in system storage forever sincehandle()never deletes anything absent from the source tree. Consider clearing/mirroring the system scope (e.g. deleteSCOPE_SYSTEMdirectories not present in the source) so the command matches its "sync" name.🤖 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 `@Modules/Core/Console/ReportsSyncSystemCommand.php` around lines 21 - 46, Update ReportsSyncSystemCommand::handle to mirror the source template tree by removing stale files and directories under ReportTemplateStorage::SCOPE_SYSTEM before or during synchronization. Preserve existing files that are present in resource_path('report-templates'), while ensuring deleted or renamed source templates no longer remain in system storage.Modules/Core/Tests/Feature/CompanyReportBuilderTest.php-72-105 (1)
72-105: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssertion embedded inside the
/* Act */block.Line 91's
$this->assertTrue($component->instance()->canSave());sits under/* Act */(opened at line 84) rather than under/* Assert */. As per coding guidelines, tests should be "Structure[d] with /* Arrange /, / Act /, and / Assert */ blocks; define all variables in the Act section before asserting."🧹 Suggested restructuring
- /* Act */ - $component = $this->testLivewire(ReportBuilder::class, [ - 'scope' => 'company', - 'type' => 'invoice', - 'slug' => 'our-invoice', - ])->assertSuccessful(); - - $this->assertTrue($component->instance()->canSave()); - - $bands = $component->get('data.bands'); + /* Act */ + $component = $this->testLivewire(ReportBuilder::class, [ + 'scope' => 'company', + 'type' => 'invoice', + 'slug' => 'our-invoice', + ])->assertSuccessful(); + + $bands = $component->get('data.bands'); $bands['footer'][] = [ 'type' => 'masonBrick', 'attrs' => ['id' => 'page_break', 'config' => []], ]; $component->set('data.bands', $bands)->call('save')->assertHasNoErrors(); /* Assert */ + $this->assertTrue($component->instance()->canSave()); $saved = $this->storage->load('company', 'our-invoice');🤖 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 `@Modules/Core/Tests/Feature/CompanyReportBuilderTest.php` around lines 72 - 105, Move the canSave() assertion from the /* Act */ section into the /* Assert */ section of it_clones_a_system_template_and_saves_the_editable_company_copy(), keeping the component setup and save operation in Act and all verification statements together under Assert.Source: Coding guidelines
Modules/Core/Tests/Feature/PdfGenerationServiceTest.php-175-178 (1)
175-178: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
->for($this->company)for scoped factory models.
Relation,Invoice, andQuotesetcompany_idmanually. Use the factory relationship pattern so tenant scoping follows the repository’s test contract.As per coding guidelines, “Use factory method pattern
->for($company)on all scoped models in tests.”Also applies to: 190-209, 240-264
🤖 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 `@Modules/Core/Tests/Feature/PdfGenerationServiceTest.php` around lines 175 - 178, Update the Relation, Invoice, and Quote factory setups in this test to use ->for($this->company) instead of manually assigning company_id; remove the redundant company_id fields while preserving other attributes such as company_name.Source: Coding guidelines
Modules/Core/Tests/Feature/AdminReportBuilderTest.php-32-143 (1)
32-143: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winApply the required Arrange / Act / Assert structure. Keep phase labels separate and prepare values before entering the Assert phase.
Modules/Core/Tests/Feature/AdminReportBuilderTest.php#L32-L143: replace the combined/* Act & Assert */marker and read assertion inputs during Act.Modules/Core/Tests/Feature/PdfGenerationServiceTest.php#L129-L161: move the early HTML assertions into the Assert phase.Modules/Core/Tests/Feature/ReportsSyncSystemCommandTest.php#L22-L47: initialize$diskand$storageduring Act rather than Assert.Modules/Core/Tests/Unit/BrowsershotDriverTest.php#L26-L39: add a distinct Act block for PDF generation before the Assert block.As per coding guidelines, “Structure tests with
/* Arrange */,/* Act */, and/* Assert */blocks; define all variables in the Act section before asserting.”🤖 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 `@Modules/Core/Tests/Feature/AdminReportBuilderTest.php` around lines 32 - 143, Restructure the affected tests to use distinct Arrange, Act, and Assert phases. In Modules/Core/Tests/Feature/AdminReportBuilderTest.php lines 32-143, replace the combined Act & Assert marker and retrieve assertion inputs during Act; in Modules/Core/Tests/Feature/PdfGenerationServiceTest.php lines 129-161, move early HTML assertions into Assert; in Modules/Core/Tests/Feature/ReportsSyncSystemCommandTest.php lines 22-47, initialize $disk and $storage during Act; and in Modules/Core/Tests/Unit/BrowsershotDriverTest.php lines 26-39, add an Act phase for PDF generation before Assert.Source: Coding guidelines
🧹 Nitpick comments (7)
Modules/Invoices/Tests/Feature/RecurringInvoicesTest.php (1)
35-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused local assignments while preserving required factory setup.
PHPMD flags
$user,$customer, and$product. Keep the factory calls if they populate form options, but discard their return values and remove$user.Also applies to: 70-73, 110-113, 138-141, 177-180, 211-214
🤖 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 `@Modules/Invoices/Tests/Feature/RecurringInvoicesTest.php` around lines 35 - 38, Remove the unused $user, $customer, and $product assignments in the affected recurring invoice test setup blocks, while retaining the corresponding factory calls without assigning their return values so required form options remain populated; apply the same change to each repeated block.Source: Linters/SAST tools
composer.json (1)
30-30: 🩺 Stability & Availability | 🔵 TrivialProvision Browsershot’s external runtime.
The Composer package does not install Node, Puppeteer, or Chromium. Verify that CI and production images provision and pin those dependencies before enabling
IP_PDF_DRIVER=Browsershot, or PDF generation will fail at runtime.🤖 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 `@composer.json` at line 30, Provision and pin the Node.js, Puppeteer, and Chromium runtime dependencies required by spatie/browsershot in the CI and production image setup before enabling IP_PDF_DRIVER=Browsershot. Ensure the existing image/build configuration installs and validates these dependencies so PDF generation works at runtime.Modules/Core/Tests/Unit/MasonBricksTest.php (1)
241-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFollow the required test conventions.
Type the callback parameter at Line 241, and split
/* Act & Assert */into distinct/* Act */and/* Assert */phases so variables are established before assertions. As per coding guidelines, PHP must use native type hints and tests must use separate/* Arrange */,/* Act */, and/* Assert */blocks.Also applies to: 261-266, 282-286
🤖 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 `@Modules/Core/Tests/Unit/MasonBricksTest.php` at line 241, Update the affected tests around the bricks ID mapping and the referenced ranges to type the array_map callback parameter with the appropriate native brick type, and separate combined Act & Assert sections into distinct Arrange, Act, and Assert blocks. Establish test variables in Arrange or Act before performing assertions, while preserving the existing test behavior.Source: Coding guidelines
Modules/Core/Tests/Unit/ReportBricksCollectionTest.php (1)
127-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep collection calls in the Act phase.
forBand()andfindById()execute inside assertions. Store their results in each test’s/* Act */block before asserting.As per coding guidelines, “define all variables in the Act section before asserting.”
Also applies to: 139-142
🤖 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 `@Modules/Core/Tests/Unit/ReportBricksCollectionTest.php` around lines 127 - 133, Move the ReportBricksCollection::forBand() and findById() calls out of assertion expressions and into the corresponding tests’ /* Act */ sections, storing each result in a variable before the /* Assert */ phase. Keep the existing assertions and expected behavior unchanged, including the loop over ReportBand::cases().Source: Coding guidelines
Modules/Core/Services/ReportDataMapper.php (1)
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing native type hints on
$item/$model.Per the coding guideline requiring native PHP type hints throughout,
itemData($item)andcommunication($model, string $type)lack a type on their first parameter.As per coding guidelines: "Use native PHP type hints throughout the codebase."
Also applies to: 139-139
🤖 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 `@Modules/Core/Services/ReportDataMapper.php` at line 113, Update the first parameters of the ReportDataMapper methods itemData and communication to use the appropriate native PHP type hints, preserving their existing behavior and the string type hint on communication’s $type parameter.Source: Coding guidelines
Modules/Core/Services/ReportTemplateStorage.php (1)
225-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSystem-default deletion guard is slug-literal, not manifest/shipped-set driven.
delete()only blocks deletion when$slug === 'default'. Any future shipped system template using a different slug would have no protection from deletion via this method. Consider deriving protection from the shippedresources/report-templatesset (e.g. viaReportTemplateStorage::path()+File::exists()against the source tree) or aprotectedflag in the manifest, rather than a hardcoded slug string.🤖 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 `@Modules/Core/Services/ReportTemplateStorage.php` around lines 225 - 238, Update ReportTemplateStorage::delete to determine protected system templates from the shipped resources/report-templates set or the manifest’s protected flag instead of comparing only against the literal “default” slug. Preserve deletion for non-system or non-protected templates, and continue throwing RuntimeException for every protected system template.Modules/Core/Tests/Feature/AdminReportBuilderTest.php (1)
29-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAdd test group attributes consistently. Every new test method lacks the required
#[Group(...)]classification.
Modules/Core/Tests/Feature/AdminReportBuilderTest.php#L29-L144: add an appropriate Group attribute to each report-builder test.Modules/Core/Tests/Feature/PdfGenerationServiceTest.php#L29-L282: add an appropriate Group attribute to each PDF-generation test.Modules/Core/Tests/Feature/ReportsSyncSystemCommandTest.php#L19-L70: add an appropriate Group attribute to each command test.Modules/Core/Tests/Unit/BrowsershotDriverTest.php#L13-L40: add an appropriate Group attribute to each driver test.Modules/Core/Tests/Unit/DomPdfDriverTest.php#L12-L28: add an appropriate Group attribute to each driver test.Modules/Core/Tests/Unit/MasonDocumentConverterTest.php#L11-L104: add an appropriate Group attribute to each converter test.Modules/Core/Tests/Unit/ReportRendererTest.php#L20-L101: add an appropriate Group attribute to each renderer test.As per coding guidelines, “Use
#[Group('smoke|crud|security|authentication|...')]attribute to organize tests by group.”🤖 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 `@Modules/Core/Tests/Feature/AdminReportBuilderTest.php` around lines 29 - 144, Classify every test method with an appropriate PHPUnit #[Group(...)] attribute. Apply this to all report-builder tests in Modules/Core/Tests/Feature/AdminReportBuilderTest.php (lines 29-144), PDF-generation tests in Modules/Core/Tests/Feature/PdfGenerationServiceTest.php (lines 29-282), command tests in Modules/Core/Tests/Feature/ReportsSyncSystemCommandTest.php (lines 19-70), driver tests in Modules/Core/Tests/Unit/BrowsershotDriverTest.php (lines 13-40) and DomPdfDriverTest.php (lines 12-28), converter tests in Modules/Core/Tests/Unit/MasonDocumentConverterTest.php (lines 11-104), and renderer tests in Modules/Core/Tests/Unit/ReportRendererTest.php (lines 20-101), using the established group vocabulary.Source: Coding guidelines
🤖 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 `@composer.json`:
- Around line 20-26: Align the composer.json PHP and laravel/framework
constraints with the supported Laravel 11 and PHP 8.1+ stack documented in
AGENTS.md, or coordinate a complete migration by updating AGENTS.md and required
compatibility changes; do not leave the dependency constraints and repository
guidance inconsistent.
In `@Modules/Core/Filament/Pages/Reports/BaseReportTemplatesPage.php`:
- Around line 121-127: Update the rename and delete action callbacks in
BaseReportTemplatesPage to derive the allowed scope from managesSystemScope()
instead of trusting the client-supplied scope, then validate the target template
with canModify() and reject it before invoking storage()->rename or
storage()->delete. Preserve the existing mutation behavior only for templates
within the permitted scope.
In `@Modules/Core/Services/PdfGenerationService.php`:
- Around line 20-26: Move PdfGenerationService out of the Services namespace
into an appropriate PDF support or action class, preserving its existing
rendering behavior and dependencies. Do not leave it as a service unless it is
redesigned to extend BaseService and comply with the array-in/model-out
contract; update references to PdfGenerationService accordingly.
In `@Modules/Core/Services/ReportTemplateStorage.php`:
- Around line 171-200: Update clone() and the underlying save path so slug
allocation and folder creation are atomic: do not rely on uniqueSlug()’s prior
exists() check alone, and make save() fail when the generated destination
already exists rather than overwriting it. Propagate that collision to the
caller, preserving the existing clone return behavior for successful writes.
- Line 29: Make ReportTemplateStorage comply with the service-layer convention
by extending BaseService, or explicitly register it as an approved pure-file
storage exception in the service guideline and its enforcement tests. If
extending BaseService, adapt the inherited model-dependent behavior so file
storage remains functional without an Eloquent model; otherwise update the
relevant validation symbols to recognize this intentional exception.
In `@Modules/Core/Support/PDF/Drivers/Browsershot.php`:
- Around line 16-36: Update the PDF contract and its implementations, including
PDFInterface, PDFAbstract, and Browsershot methods save, getOutput, and
download, so $html and $filename use the agreed native parameter types and PDF
byte-producing methods declare their return types consistently. Apply matching
signatures across inherited declarations and overrides to avoid compatibility
conflicts.
In `@Modules/Core/Support/PDF/Drivers/domPDF.php`:
- Around line 36-50: The dompdf Options configuration must allow local company
logos stored on the public disk. In the PDF driver’s Options setup, add the
public disk root to the chroot when logoPath() returns a non-blank value, while
preserving the disabled remote-fetching behavior; also add a PDF assertion
covering a document with a non-empty company logo.
In `@Modules/Core/Tests/Feature/InvoiceTemplateSelectionTest.php`:
- Around line 104-119: Update the invoice() test helper to associate Relation,
Numbering, and Invoice factories with $this->company using the
->for($this->company) factory pattern, removing their raw company_id overrides
while preserving the remaining attributes and relationships.
In `@Modules/Core/Tests/Unit/BrowsershotDriverTest.php`:
- Around line 24-34: Replace mb_trim() with the PHP-compatible trim() call in
it_produces_pdf_bytes_from_html_when_chromium_is_available(), preserving the
existing command-path check and skip behavior.
In `@resources/css/filament/company/invoiceplane-blue.css`:
- Around line 5-7: Update the source declarations in the blue theme stylesheet
to restore the app/Filament/Tenant/**/* and resources/views/filament/tenant/**/*
globs, while retaining the existing module and Mason sources.
---
Minor comments:
In `@CLAUDE.md`:
- Around line 5-13: Update the Laravel and PHP version guidance in
.github/CONTRIBUTING.md, .github/scripts/README.md, and
.github/copilot-instructions.md to match Laravel 13 and PHP 8.3+ (PHP ^8.3),
including any related upgrade or tooling instructions. Keep the existing
project-specific guidance intact and do not change CLAUDE.md.
In `@Modules/Core/Console/ReportsSyncSystemCommand.php`:
- Around line 21-46: Update ReportsSyncSystemCommand::handle to mirror the
source template tree by removing stale files and directories under
ReportTemplateStorage::SCOPE_SYSTEM before or during synchronization. Preserve
existing files that are present in resource_path('report-templates'), while
ensuring deleted or renamed source templates no longer remain in system storage.
In `@Modules/Core/Mason/Bricks/DetailQuoteProjectBrick.php`:
- Line 24: Apply Pint’s union-type formatting to getIcon in
Modules/Core/Mason/Bricks/DetailQuoteProjectBrick.php:24-24,
Modules/Core/Mason/Bricks/DetailTasksBrick.php:25-25, and
Modules/Core/Mason/Bricks/SpacerBrick.php:24-24 by removing spaces around each |
in the string|Htmlable|null return type.
In `@Modules/Core/Mason/Bricks/FooterTermsBrick.php`:
- Around line 57-66: Sanitize the rich HTML produced by the terms_content field
in FooterTermsBrick before it reaches report rendering, removing script and
other executable/content-dangerous tags while preserving supported formatting.
Ensure index.blade.php and any default template path do not emit unsanitized
terms_content as raw HTML, including when the Browsershot driver is used.
In `@Modules/Core/Services/ReportDataMapper.php`:
- Around line 27-33: Update the invoice mapping in
ReportDataMapper::forInvoice() so po_number uses the invoice’s real reference
field, preferring client_reference or work_order according to the domain model,
instead of always returning an empty string. Preserve the existing output key
and formatting so HeaderInvoiceMetaBrick can display the configured PO/reference
value.
In `@Modules/Core/Tests/Feature/AdminReportBuilderTest.php`:
- Around line 32-143: Restructure the affected tests to use distinct Arrange,
Act, and Assert phases. In Modules/Core/Tests/Feature/AdminReportBuilderTest.php
lines 32-143, replace the combined Act & Assert marker and retrieve assertion
inputs during Act; in Modules/Core/Tests/Feature/PdfGenerationServiceTest.php
lines 129-161, move early HTML assertions into Assert; in
Modules/Core/Tests/Feature/ReportsSyncSystemCommandTest.php lines 22-47,
initialize $disk and $storage during Act; and in
Modules/Core/Tests/Unit/BrowsershotDriverTest.php lines 26-39, add an Act phase
for PDF generation before Assert.
In `@Modules/Core/Tests/Feature/CompanyReportBuilderTest.php`:
- Around line 72-105: Move the canSave() assertion from the /* Act */ section
into the /* Assert */ section of
it_clones_a_system_template_and_saves_the_editable_company_copy(), keeping the
component setup and save operation in Act and all verification statements
together under Assert.
In `@Modules/Core/Tests/Feature/PdfGenerationServiceTest.php`:
- Around line 175-178: Update the Relation, Invoice, and Quote factory setups in
this test to use ->for($this->company) instead of manually assigning company_id;
remove the redundant company_id fields while preserving other attributes such as
company_name.
In `@Modules/Core/Tests/Unit/MasonBricksTest.php`:
- Around line 231-238: The brick lists used by the ID, label, and icon tests in
the relevant test methods must cover every class registered by
ReportBricksCollection, including DetailInvoiceProjectBrick,
DetailQuoteProductBrick, FooterSummaryBrick, and any other registered bricks.
Extend the shared expected lists or derive them from the collection so all three
assertions validate the complete registered brick set.
In `@Modules/Core/Tests/Unit/ReportBrickTest.php`:
- Around line 42-44: Update the affected tests in ReportBrickTest so the Act
phase first materializes each brick’s configKeys and render results, then the
Assert phase validates those stored values. Keep brick method execution out of
the assertions while preserving the existing coverage and failure messages.
In `@resources/lang/en/ip.php`:
- Line 1230: Remove the later duplicate pdf_template entry from the translation
array in ip.php, preserving the earlier existing definition and its value.
---
Nitpick comments:
In `@composer.json`:
- Line 30: Provision and pin the Node.js, Puppeteer, and Chromium runtime
dependencies required by spatie/browsershot in the CI and production image setup
before enabling IP_PDF_DRIVER=Browsershot. Ensure the existing image/build
configuration installs and validates these dependencies so PDF generation works
at runtime.
In `@Modules/Core/Services/ReportDataMapper.php`:
- Line 113: Update the first parameters of the ReportDataMapper methods itemData
and communication to use the appropriate native PHP type hints, preserving their
existing behavior and the string type hint on communication’s $type parameter.
In `@Modules/Core/Services/ReportTemplateStorage.php`:
- Around line 225-238: Update ReportTemplateStorage::delete to determine
protected system templates from the shipped resources/report-templates set or
the manifest’s protected flag instead of comparing only against the literal
“default” slug. Preserve deletion for non-system or non-protected templates, and
continue throwing RuntimeException for every protected system template.
In `@Modules/Core/Tests/Feature/AdminReportBuilderTest.php`:
- Around line 29-144: Classify every test method with an appropriate PHPUnit
#[Group(...)] attribute. Apply this to all report-builder tests in
Modules/Core/Tests/Feature/AdminReportBuilderTest.php (lines 29-144),
PDF-generation tests in Modules/Core/Tests/Feature/PdfGenerationServiceTest.php
(lines 29-282), command tests in
Modules/Core/Tests/Feature/ReportsSyncSystemCommandTest.php (lines 19-70),
driver tests in Modules/Core/Tests/Unit/BrowsershotDriverTest.php (lines 13-40)
and DomPdfDriverTest.php (lines 12-28), converter tests in
Modules/Core/Tests/Unit/MasonDocumentConverterTest.php (lines 11-104), and
renderer tests in Modules/Core/Tests/Unit/ReportRendererTest.php (lines 20-101),
using the established group vocabulary.
In `@Modules/Core/Tests/Unit/MasonBricksTest.php`:
- Line 241: Update the affected tests around the bricks ID mapping and the
referenced ranges to type the array_map callback parameter with the appropriate
native brick type, and separate combined Act & Assert sections into distinct
Arrange, Act, and Assert blocks. Establish test variables in Arrange or Act
before performing assertions, while preserving the existing test behavior.
In `@Modules/Core/Tests/Unit/ReportBricksCollectionTest.php`:
- Around line 127-133: Move the ReportBricksCollection::forBand() and findById()
calls out of assertion expressions and into the corresponding tests’ /* Act */
sections, storing each result in a variable before the /* Assert */ phase. Keep
the existing assertions and expected behavior unchanged, including the loop over
ReportBand::cases().
In `@Modules/Invoices/Tests/Feature/RecurringInvoicesTest.php`:
- Around line 35-38: Remove the unused $user, $customer, and $product
assignments in the affected recurring invoice test setup blocks, while retaining
the corresponding factory calls without assigning their return values so
required form options remain populated; apply the same change to each repeated
block.
🪄 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: f22f9ccf-7a51-4ff7-a0d9-8d7967a124fe
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (97)
.gitignoreCLAUDE.mdModules/Core/Console/ReportsSyncSystemCommand.phpModules/Core/Database/Factories/CompanyUserFactory.phpModules/Core/Database/Factories/CustomFieldValueFactory.phpModules/Core/Database/Factories/NoteFactory.phpModules/Core/Enums/ReportBand.phpModules/Core/Enums/ReportBlockWidth.phpModules/Core/Enums/ReportTemplateType.phpModules/Core/Filament/Admin/Pages/ReportBuilder.phpModules/Core/Filament/Admin/Pages/ReportTemplates.phpModules/Core/Filament/Company/Pages/ReportBuilder.phpModules/Core/Filament/Company/Pages/ReportTemplates.phpModules/Core/Filament/Pages/Reports/BaseReportBuilderPage.phpModules/Core/Filament/Pages/Reports/BaseReportTemplatesPage.phpModules/Core/Mason/Bricks/DetailCustomerAgingBrick.phpModules/Core/Mason/Bricks/DetailExpenseBrick.phpModules/Core/Mason/Bricks/DetailInvoiceProductBrick.phpModules/Core/Mason/Bricks/DetailInvoiceProjectBrick.phpModules/Core/Mason/Bricks/DetailItemsBrick.phpModules/Core/Mason/Bricks/DetailQuoteProductBrick.phpModules/Core/Mason/Bricks/DetailQuoteProjectBrick.phpModules/Core/Mason/Bricks/DetailTasksBrick.phpModules/Core/Mason/Bricks/FooterNotesBrick.phpModules/Core/Mason/Bricks/FooterSummaryBrick.phpModules/Core/Mason/Bricks/FooterTermsBrick.phpModules/Core/Mason/Bricks/FooterTotalsBrick.phpModules/Core/Mason/Bricks/HeaderClientBrick.phpModules/Core/Mason/Bricks/HeaderCompanyBrick.phpModules/Core/Mason/Bricks/HeaderInvoiceMetaBrick.phpModules/Core/Mason/Bricks/HeaderProjectBrick.phpModules/Core/Mason/Bricks/HeaderQuoteMetaBrick.phpModules/Core/Mason/Bricks/PageBreakBrick.phpModules/Core/Mason/Bricks/SpacerBrick.phpModules/Core/Mason/MasonDocumentConverter.phpModules/Core/Mason/ReportBrick.phpModules/Core/Mason/ReportBricksCollection.phpModules/Core/Models/Company.phpModules/Core/Providers/AdminPanelProvider.phpModules/Core/Providers/CompanyPanelProvider.phpModules/Core/Providers/CoreServiceProvider.phpModules/Core/Services/PdfGenerationService.phpModules/Core/Services/ReportDataMapper.phpModules/Core/Services/ReportRenderer.phpModules/Core/Services/ReportTemplateStorage.phpModules/Core/Support/PDF/Drivers/Browsershot.phpModules/Core/Support/PDF/Drivers/domPDF.phpModules/Core/Tests/Feature/AdminReportBuilderTest.phpModules/Core/Tests/Feature/CompanyReportBuilderTest.phpModules/Core/Tests/Feature/InvoiceTemplateSelectionTest.phpModules/Core/Tests/Feature/PdfGenerationServiceTest.phpModules/Core/Tests/Feature/ReportsSyncSystemCommandTest.phpModules/Core/Tests/Feature/Seeders/RoleHasPermissionsSeederTest.phpModules/Core/Tests/Fixtures/report-templates/invoice-default.htmlModules/Core/Tests/Fixtures/report-templates/quote-default.htmlModules/Core/Tests/Unit/BrowsershotDriverTest.phpModules/Core/Tests/Unit/DomPdfDriverTest.phpModules/Core/Tests/Unit/MasonBricksTest.phpModules/Core/Tests/Unit/MasonDocumentConverterTest.phpModules/Core/Tests/Unit/ReportBrickTest.phpModules/Core/Tests/Unit/ReportBricksCollectionTest.phpModules/Core/Tests/Unit/ReportRendererTest.phpModules/Core/Tests/Unit/ReportTemplateStorageTest.phpModules/Core/resources/views/filament/pages/reports/report-builder.blade.phpModules/Core/resources/views/filament/pages/reports/report-templates.blade.phpModules/Expenses/Filament/Company/Widgets/RecentExpensesWidget.phpModules/Expenses/Observers/ExpenseObserver.phpModules/Invoices/Filament/Company/Resources/Invoices/Schemas/InvoiceForm.phpModules/Invoices/Filament/Company/Resources/Invoices/Tables/InvoicesTable.phpModules/Invoices/Tests/Feature/InvoiceDuplicateNumberPreventionTest.phpModules/Invoices/Tests/Feature/InvoicePdfAndCreditNoteTest.phpModules/Invoices/Tests/Feature/RecurringInvoicesTest.phpModules/Payments/Filament/Company/Widgets/RecentPaymentsWidget.phpModules/Payments/Observers/PaymentObserver.phpModules/Projects/Filament/Company/Widgets/RecentProjectsWidget.phpModules/Projects/Filament/Company/Widgets/RecentTasksWidget.phpModules/Quotes/Filament/Company/Resources/Quotes/Schemas/QuoteForm.phpModules/Quotes/Filament/Company/Resources/Quotes/Tables/QuotesTable.phpModules/Quotes/Tests/Feature/QuoteDuplicateNumberPreventionTest.phpModules/Quotes/Tests/Unit/QuoteModelTest.phpcomposer.jsonconfig/ip.phpresources/css/filament/company/invoiceplane-blue.cssresources/css/filament/company/invoiceplane.cssresources/css/filament/company/nord.cssresources/css/filament/company/orange.cssresources/css/filament/company/reddit.cssresources/lang/en/ip.phpresources/report-templates/invoice/default/bands.jsonresources/report-templates/invoice/default/manifest.jsonresources/report-templates/quote/default/bands.jsonresources/report-templates/quote/default/manifest.jsonresources/views/mason/bricks/header-company/index.blade.phpresources/views/mason/bricks/page-break/index.blade.phpresources/views/mason/bricks/page-break/preview.blade.phpresources/views/mason/bricks/spacer/index.blade.phpresources/views/mason/bricks/spacer/preview.blade.php
💤 Files with no reviewable changes (2)
- Modules/Invoices/Tests/Feature/InvoiceDuplicateNumberPreventionTest.php
- Modules/Quotes/Tests/Feature/QuoteDuplicateNumberPreventionTest.php
Comment on lines
+20
to
+26
| "php": "^8.3", | ||
| "awcodes/mason": "^3.1", | ||
| "doctrine/dbal": ">=4.4", | ||
| "dompdf/dompdf": "^3.1", | ||
| "filament/actions": ">=5.6", | ||
| "filament/filament": ">=5.6", | ||
| "laravel/framework": ">=12.46", | ||
| "laravel/framework": "^13.0", |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== composer relevant sections =="
python3 - <<'PY'
import json
p='composer.json'
try:
data=json.load(open(p))
except Exception as e:
print(f"cannot parse {p}: {e}")
raise
for key in ["require","require-dev","config","scripts","platform"]:
if key in data:
print(f"-- {key} --")
if isinstance(data[key], dict):
for k,v in sorted(data[key].items()):
if key in ("require","require-dev","config.platform"):
print(f"{k}: {v}")
else:
print(data[key])
PY
echo
echo "== Laravel 13 availability / composer.lock framework version? =="
if [ -f composer.lock ]; then
python3 - <<'PY'
import json
data=json.load(open('composer.lock'))
for pkg in data.get('packages', []) + data.get('packages-dev', []):
if pkg.get('name') == 'laravel/framework':
print(pkg.get('name'), pkg.get('version'), pkg.get('source', {}).get('reference'))
PY
else
echo "no composer.lock"
fi
echo
echo "== repository mentions of supported stack/Laravel 11/PHP =="
rg -n --hidden --glob '!vendor/**' --glob '!node_modules/**' 'Laravel 11|PHP 8\.1|8\.3|composer\.json|PHP|Laravel' -S . | head -200Repository: InvoicePlane/InvoicePlane-v2
Length of output: 21417
Keep Composer aligned with the repository’s supported stack.
composer.json now requires PHP ^8.3 and Laravel ^13.0, while AGENTS.md documents this as a Laravel 11 + PHP 8.1+ app. Either keep the supported constraints or update the repository guidance and compatibility work in a coordinated migration.
🤖 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 `@composer.json` around lines 20 - 26, Align the composer.json PHP and
laravel/framework constraints with the supported Laravel 11 and PHP 8.1+ stack
documented in AGENTS.md, or coordinate a complete migration by updating
AGENTS.md and required compatibility changes; do not leave the dependency
constraints and repository guidance inconsistent.
Source: Coding guidelines
Comment on lines
+121
to
+127
| ->action(function (array $arguments, array $data): void { | ||
| $this->storage()->rename( | ||
| (string) $arguments['scope'], | ||
| (string) $arguments['slug'], | ||
| (string) $data['name'], | ||
| ReportTemplateType::tryFrom((string) $arguments['type']), | ||
| ); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce the editable scope inside rename and delete actions.
scope is client-supplied, while canModify() is only a UI predicate. A company-panel caller can invoke these actions with scope=system and rename or delete shared templates. Derive the permitted scope from managesSystemScope() and reject templates that fail canModify() before storage mutation.
Also applies to: 140-145
🤖 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 `@Modules/Core/Filament/Pages/Reports/BaseReportTemplatesPage.php` around lines
121 - 127, Update the rename and delete action callbacks in
BaseReportTemplatesPage to derive the allowed scope from managesSystemScope()
instead of trusting the client-supplied scope, then validate the target template
with canModify() and reject it before invoking storage()->rename or
storage()->delete. Preserve the existing mutation behavior only for templates
within the permitted scope.
Comment on lines
+20
to
+26
| class PdfGenerationService | ||
| { | ||
| public function __construct( | ||
| protected ReportTemplateStorage $storage, | ||
| protected ReportRenderer $renderer, | ||
| protected ReportDataMapper $mapper, | ||
| ) {} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move this output generator out of Services/, or conform to the service contract.
Its public API accepts Invoice/Quote models and returns PDF bytes or HTTP responses; it neither extends BaseService nor uses the required array-in/model-out contract. Move it to a PDF support/action class, or redesign the API consistently.
As per coding guidelines, “All service classes must extend Modules\Core\Services\BaseService, accept arrays rather than DTOs, and return Eloquent models.”
🤖 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 `@Modules/Core/Services/PdfGenerationService.php` around lines 20 - 26, Move
PdfGenerationService out of the Services namespace into an appropriate PDF
support or action class, preserving its existing rendering behavior and
dependencies. Do not leave it as a service unless it is redesigned to extend
BaseService and comply with the array-in/model-out contract; update references
to PdfGenerationService accordingly.
Source: Coding guidelines
Comment on lines
+171
to
+200
| public function clone( | ||
| string $fromScope, | ||
| string $fromSlug, | ||
| string $newName, | ||
| ?ReportTemplateType $type = null, | ||
| string $toScope = self::SCOPE_COMPANY, | ||
| ): array { | ||
| $source = $this->load($fromScope, $fromSlug, $type); | ||
|
|
||
| if ($source === null) { | ||
| throw new RuntimeException("Report template [{$fromScope}/{$fromSlug}] does not exist."); | ||
| } | ||
|
|
||
| $manifest = $source['manifest']; | ||
| $templateType = $type ?? ReportTemplateType::tryFrom((string) ($manifest['type'] ?? '')); | ||
| $newSlug = $this->uniqueSlug($toScope, Str::slug($newName), $templateType); | ||
|
|
||
| $manifest['name'] = $newName; | ||
| $manifest['slug'] = $newSlug; | ||
| $manifest['cloned_from'] = $this->path($fromScope, $fromSlug, $type); | ||
|
|
||
| $this->save($toScope, $newSlug, $manifest, $source['bands'], $templateType); | ||
|
|
||
| return [ | ||
| 'scope' => $toScope, | ||
| 'type' => (string) ($manifest['type'] ?? ''), | ||
| 'slug' => $newSlug, | ||
| 'manifest' => $manifest, | ||
| ]; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚖️ Poor tradeoff
TOCTOU race between uniqueSlug()'s existence check and save()'s write.
clone() calls uniqueSlug() (check exists() in a loop) and only afterwards calls save() to write the folder. Two concurrent clone/save requests for the same generated slug (e.g. a double-submitted "Copy" action) can both pass the existence check before either write lands, so one clone silently overwrites the other's manifest/bands with no error surfaced to either caller.
Also applies to: 339-351
🤖 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 `@Modules/Core/Services/ReportTemplateStorage.php` around lines 171 - 200,
Update clone() and the underlying save path so slug allocation and folder
creation are atomic: do not rely on uniqueSlug()’s prior exists() check alone,
and make save() fail when the generated destination already exists rather than
overwriting it. Propagate that collision to the caller, preserving the existing
clone return behavior for successful writes.
Comment on lines
+16
to
+36
| public function save($html, $filename): void | ||
| { | ||
| file_put_contents($filename, $this->getOutput($html)); | ||
| } | ||
|
|
||
| public function getOutput($html) | ||
| { | ||
| return $this->getEngine($html)->pdf(); | ||
| } | ||
|
|
||
| public function download($html, $filename) | ||
| { | ||
| $response = response($this->getOutput($html)); | ||
|
|
||
| $response->header('Content-Type', 'application/pdf'); | ||
| $response->header('Content-Disposition', 'attachment; filename="' . $filename . '"'); | ||
|
|
||
| return $response->send(); | ||
| } | ||
|
|
||
| protected function getEngine($html): BrowsershotEngine |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^PDFAbstract\.php$' . -x ast-grep outline {} --items all
fd -a '^PDFAbstract\.php$' . -x nl -ba {}
nl -ba Modules/Core/Support/PDF/Drivers/Browsershot.php | sed -n '12,72p'
fd -a '^(domPDF|Browsershot)\.php$' Modules/Core/Support/PDF/Drivers -x ast-grep outline {} --items allRepository: InvoicePlane/InvoicePlane-v2
Length of output: 422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'PDFFile outlines:\n'
fd -a 'PDFAbstract\.php|PDFInterface\.php|.*\.php$' Modules/Core/Support/PDF -x sh -c 'echo "--- $1"; ast-grep outline "$1" --items all || true' sh {}
printf '\nPDF files relevant content:\n'
for f in Modules/Core/Support/PDF/PDFInterface.php Modules/Core/Support/PDF/PDFAbstract.php Modules/Core/Support/PDF/Drivers/Browsershot.php Modules/Core/Support/PDF/Drivers/domPDF.php Modules/Core/Support/PDF/Drivers/Browsershot.php; do
if [ -f "$f" ]; then
echo "===== $f ====="
sed -n '1,140p' "$f"
fi
done
printf '\nPHP parser compatibility probe for current Browsershot overrides vs static signature if any:\n'
python3 - <<'PY'
import re, pathlib
for path in [
pathlib.Path("Modules/Core/Support/PDF/PDFInterface.php"),
pathlib.Path("Modules/Core/Support/PDF/PDFAbstract.php"),
pathlib.Path("Modules/Core/Support/PDF/Drivers/Browsershot.php"),
]:
print(f"** {path} **")
text = path.read_text()
for name in ["save", "getOutput", "download"]:
for m in re.finditer(rf'function\s+{re.escape(name)}\s*\((.*?)\)\s*:\s*([<>\w\|\&/,\s\.]+|\[\])', text, re.S):
print(name, "params=", m.group(1).strip(), "return=", m.group(2).strip())
PYRepository: InvoicePlane/InvoicePlane-v2
Length of output: 9918
Make the PDF driver signatures consistently typed across the contract.
PDFAbstract implements PDFInterface, but $html, $filename, and PDF bytes are still untyped. Narrowing signatures only on Browsershot::save() would still clash with the inherited non-typed PDFInterface/ancestor signatures. Add the same native parameter/return types consistently to the contract and driver overrides instead.
🤖 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 `@Modules/Core/Support/PDF/Drivers/Browsershot.php` around lines 16 - 36,
Update the PDF contract and its implementations, including PDFInterface,
PDFAbstract, and Browsershot methods save, getOutput, and download, so $html and
$filename use the agreed native parameter types and PDF byte-producing methods
declare their return types consistently. Apply matching signatures across
inherited declarations and overrides to avoid compatibility conflicts.
Source: Coding guidelines
Comment on lines
+36
to
+50
| $workDir = storage_path('app/dompdf'); | ||
| File::ensureDirectoryExists($workDir); | ||
|
|
||
| $options = new Options(); | ||
|
|
||
| $options->setTempDir(storage_path('/')); | ||
| $options->setFontDir(storage_path('/')); | ||
| $options->setFontCache(storage_path('/')); | ||
| $options->setLogOutputFile(storage_path('dompdf_log')); | ||
| $options->setIsRemoteEnabled(true); | ||
| $options->setTempDir($workDir); | ||
| $options->setFontDir($workDir); | ||
| $options->setFontCache($workDir); | ||
| $options->setLogOutputFile($workDir . '/dompdf.log'); | ||
| // Remote fetching stays disabled: images must resolve to local paths. | ||
| $options->setIsRemoteEnabled(false); | ||
| $options->setIsHtml5ParserEnabled(true); | ||
| $options->setIsFontSubsettingEnabled(true); | ||
|
|
||
| $pdf = new PDF($options); | ||
| $pdf = new DompdfEngine($options); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 --glob '*.php' \
'setChroot|setIsRemoteEnabled|function logoPath|Storage::disk\(.public.\)->path' \
Modules/Core
fd -a '^DomPdfDriverTest\.php$' Modules -x ast-grep outline {} --items all
fd -a '^DomPdfDriverTest\.php$' Modules -x sed -n '1,180p' {}Repository: InvoicePlane/InvoicePlane-v2
Length of output: 2908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- tracked dompdf-related files ---\n'
git ls-files | rg -i 'dompdf|pdf|report' | sed -n '1,120p'
printf '\n--- domPDF driver ---\n'
cat -n Modules/Core/Support/PDF/Drivers/domPDF.php | sed -n '1,90p'
printf '\n--- ReportDataMapper relevant methods ---\n'
cat -n Modules/Core/Services/ReportDataMapper.php | sed -n '1,180p'
printf '\n--- DomPdfDriverTest ---\n'
cat -n Modules/Core/Tests/Unit/DomPdfDriverTest.php | sed -n '1,220p'
printf '\n--- composer dompdf package versions/config ---\n'
for f in composer.json composer.lock Modules/Core/composer.json Modules/Core/composer.json.dist; do
[ -f "$f" ] && printf '\n### %s ###\n' "$f" && rg -n '"dompdf|dompdf/dompdf|pdf' "$f" || true
doneRepository: InvoicePlane/InvoicePlane-v2
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked dompdf-related files ---'
git ls-files | rg -i 'dompdf|pdf|report' | sed -n '1,160p'
printf '%s\n' ''
printf '%s\n' '--- domPDF driver ---'
cat -n Modules/Core/Support/PDF/Drivers/domPDF.php | sed -n '1,120p'
printf '%s\n' ''
printf '%s\n' '--- ReportDataMapper relevant methods ---'
cat -n Modules/Core/Services/ReportDataMapper.php | sed -n '1,190p'
printf '%s\n' ''
printf '%s\n' '--- DomPdfDriverTest ---'
cat -n Modules/Core/Tests/Unit/DomPdfDriverTest.php | sed -n '1,240p'
printf '%s\n' ''
printf '%s\n' '--- composer dompdf package versions/config ---'
for f in composer.json composer.lock Modules/Core/composer.json Modules/Core/composer.json.dist; do
if [ -f "$f" ]; then
printf '%s\n' "### $f ###"
rg -n '"dompdf|dompdf/dompdf|pdf|autoload|psr-4|vendor' "$f" || true
fi
doneRepository: InvoicePlane/InvoicePlane-v2
Length of output: 26621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dompdf v3.1.5 Options chroot/remote-local source check ---'
python3 - <<'PY'
import json, urllib.request, re
lock = json.load(open('composer.lock'))
pkg = next(p for p in lock['packages'] if p['name'] == 'dompdf/dompdf')
tag = re.search(r'v(\d+\.\d+\.\d+)', pkg['version'] or pkg['source']['reference']).group(1)
base = 'https://raw.githubusercontent.com/dompdf/dompdf/' + pkg['source']['reference']
files = {
'Options.php': f'{base}/src/Options.php',
'CanvasFactory.php': f'{base}/src/Canvas/CanvasFactory.php',
'PDF.php': f'{base}/src/Dompdf.php',
}
for name, url in files.items():
text = urllib.request.urlopen(url, timeout=20).read().decode()
hits = [(i+1,l) for i,l in enumerate(text.splitlines()) if re.search(r'(chroot|is_remote_enabled|setChroot|getChroot|isRemoteEnabled|setRemoteEnabled|public disk|local resource|allowed local)', l, re.I)]
print(f'### {name} ({len(hits)} hits) ###')
if hits:
for i,l in hits[:20]:
print(f'{i}: {l.strip()}')
PY
printf '%s\n' '--- public disk config ---'
for f in config/filesystems.php .env.example .env; do
[ -f "$f" ] && { echo "### $f ###"; rg -n -C 4 "'public'|storage_path|app/public|disk=' |path=' " "$f" || true; }
done
printf '%s\n' '--- Report template references to company logo ---'
rg -n -C 3 'company\.logo_path|logo_path|company\.logo|company_logo|<img|<image' Modules/Core/resources views resources config public storage -g '*' || trueRepository: InvoicePlane/InvoicePlane-v2
Length of output: 2824
Configure dompdf’s chroot for logos from the public disk.
logoPath() can return an absolute Storage::disk('public')->path() value, but the Options instance only sets temp/font/latency paths; remote fetching remains disabled and Dompdf will reject local image paths outside its active chroot. Set setChroot() to include the public disk root for non-blank logos and add a PDF assertion for a document that contains a non-empty company logo.
🤖 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 `@Modules/Core/Support/PDF/Drivers/domPDF.php` around lines 36 - 50, The dompdf
Options configuration must allow local company logos stored on the public disk.
In the PDF driver’s Options setup, add the public disk root to the chroot when
logoPath() returns a non-blank value, while preserving the disabled
remote-fetching behavior; also add a PDF assertion covering a document with a
non-empty company logo.
Comment on lines
+104
to
+119
| protected function invoice(): Invoice | ||
| { | ||
| $relation = Relation::factory()->create(['company_id' => $this->company->id]); | ||
| $numbering = \Modules\Core\Models\Numbering::factory()->create([ | ||
| 'company_id' => $this->company->id, | ||
| 'type' => \Modules\Core\Enums\NumberingType::INVOICE->value, | ||
| ]); | ||
|
|
||
| return Invoice::factory()->create([ | ||
| 'company_id' => $this->company->id, | ||
| 'customer_id' => $relation->id, | ||
| 'user_id' => $this->user->id, | ||
| 'numbering_id' => $numbering->id, | ||
| 'template' => null, | ||
| ]); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use ->for($company) instead of raw company_id overrides.
As per coding guidelines, tests should "Use factory method pattern ->for($company) on all scoped models in tests." Relation, Numbering, and Invoice are all created with ['company_id' => $this->company->id] array overrides instead.
♻️ Suggested fix
- $relation = Relation::factory()->create(['company_id' => $this->company->id]);
- $numbering = \Modules\Core\Models\Numbering::factory()->create([
- 'company_id' => $this->company->id,
- 'type' => \Modules\Core\Enums\NumberingType::INVOICE->value,
- ]);
-
- return Invoice::factory()->create([
- 'company_id' => $this->company->id,
- 'customer_id' => $relation->id,
+ $relation = Relation::factory()->for($this->company)->create();
+ $numbering = \Modules\Core\Models\Numbering::factory()->for($this->company)->create([
+ 'type' => \Modules\Core\Enums\NumberingType::INVOICE->value,
+ ]);
+
+ return Invoice::factory()->for($this->company)->create([
+ 'customer_id' => $relation->id,
'user_id' => $this->user->id,
'numbering_id' => $numbering->id,
'template' => 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
| protected function invoice(): Invoice | |
| { | |
| $relation = Relation::factory()->create(['company_id' => $this->company->id]); | |
| $numbering = \Modules\Core\Models\Numbering::factory()->create([ | |
| 'company_id' => $this->company->id, | |
| 'type' => \Modules\Core\Enums\NumberingType::INVOICE->value, | |
| ]); | |
| return Invoice::factory()->create([ | |
| 'company_id' => $this->company->id, | |
| 'customer_id' => $relation->id, | |
| 'user_id' => $this->user->id, | |
| 'numbering_id' => $numbering->id, | |
| 'template' => null, | |
| ]); | |
| } | |
| protected function invoice(): Invoice | |
| { | |
| $relation = Relation::factory()->for($this->company)->create(); | |
| $numbering = \Modules\Core\Models\Numbering::factory()->for($this->company)->create([ | |
| 'type' => \Modules\Core\Enums\NumberingType::INVOICE->value, | |
| ]); | |
| return Invoice::factory()->for($this->company)->create([ | |
| 'customer_id' => $relation->id, | |
| 'user_id' => $this->user->id, | |
| 'numbering_id' => $numbering->id, | |
| 'template' => 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 `@Modules/Core/Tests/Feature/InvoiceTemplateSelectionTest.php` around lines 104
- 119, Update the invoice() test helper to associate Relation, Numbering, and
Invoice factories with $this->company using the ->for($this->company) factory
pattern, removing their raw company_id overrides while preserving the remaining
attributes and relationships.
Source: Coding guidelines
Comment on lines
+24
to
+34
| public function it_produces_pdf_bytes_from_html_when_chromium_is_available(): void | ||
| { | ||
| /* Arrange — opt-in driver: skip on hosts without Node/Chromium */ | ||
| if (mb_trim((string) shell_exec('command -v node 2>/dev/null')) === '') { | ||
| $this->markTestSkipped('Node is not available on this host.'); | ||
| } | ||
|
|
||
| try { | ||
| $output = (new Browsershot())->getOutput('<p>Hello Chromium PDF</p>'); | ||
| } catch (Throwable $e) { | ||
| $this->markTestSkipped('Chromium/Puppeteer is not available: ' . mb_substr($e->getMessage(), 0, 120)); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
grep -nE '"php"[[:space:]]*:' composer.json || true
rg -n 'mb_trim\s*\(' Modules/Core/Tests/Unit/BrowsershotDriverTest.phpRepository: InvoicePlane/InvoicePlane-v2
Length of output: 277
🌐 Web query:
PHP mb_trim function added version mbstring trim PHP documentation
💡 Result:
The mb_trim function was added to the PHP mbstring extension in PHP 8.4.0 [1][2]. It provides a multi-byte safe alternative to the standard trim function, allowing for the stripping of whitespace or specified characters from the beginning and end of a string while respecting multi-byte character encodings [1][3]. Official PHP documentation confirms its availability starting from PHP 8.4 [1][4].
Citations:
- 1: https://www.php.net/manual/en/function.mb-trim.php
- 2: https://php.watch/versions/8.4/mb_trim-mb_ltrim-mb_rtrim
- 3: https://mirror.math.princeton.edu/pub/php/manual/en/function.mb-trim.php
- 4: https://www.php.net/mb_trim
Replace mb_trim() with a PHP-compatible call.
mb_trim() is only available in PHP 8.4+, while this repository requires PHP ^8.3; this test will fatal on supported PHP 8.3 hosts before the Node/Chromium skip can run. trim() is sufficient for the command-path check.
Proposed fix
- if (mb_trim((string) shell_exec('command -v node 2>/dev/null')) === '') {
+ if (trim((string) shell_exec('command -v node 2>/dev/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
| public function it_produces_pdf_bytes_from_html_when_chromium_is_available(): void | |
| { | |
| /* Arrange — opt-in driver: skip on hosts without Node/Chromium */ | |
| if (mb_trim((string) shell_exec('command -v node 2>/dev/null')) === '') { | |
| $this->markTestSkipped('Node is not available on this host.'); | |
| } | |
| try { | |
| $output = (new Browsershot())->getOutput('<p>Hello Chromium PDF</p>'); | |
| } catch (Throwable $e) { | |
| $this->markTestSkipped('Chromium/Puppeteer is not available: ' . mb_substr($e->getMessage(), 0, 120)); | |
| public function it_produces_pdf_bytes_from_html_when_chromium_is_available(): void | |
| { | |
| /* Arrange — opt-in driver: skip on hosts without Node/Chromium */ | |
| if (trim((string) shell_exec('command -v node 2>/dev/null')) === '') { | |
| $this->markTestSkipped('Node is not available on this host.'); | |
| } | |
| try { | |
| $output = (new Browsershot())->getOutput('<p>Hello Chromium PDF</p>'); | |
| } catch (Throwable $e) { | |
| $this->markTestSkipped('Chromium/Puppeteer is not available: ' . mb_substr($e->getMessage(), 0, 120)); |
🤖 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 `@Modules/Core/Tests/Unit/BrowsershotDriverTest.php` around lines 24 - 34,
Replace mb_trim() with the PHP-compatible trim() call in
it_produces_pdf_bytes_from_html_when_chromium_is_available(), preserving the
existing command-path check and skip behavior.
Source: Coding guidelines
Local tests silently diverging from CI's MariaDB (via a documented SQLite .env.testing fallback) has repeatedly masked real bugs this session — ->latest() defaulting to a nonexistent created_at column, and identifier quoting differences, both passed locally on SQLite and only failed on CI. - docker-compose.yml: cli service now injects DB_CONNECTION=mysql/DB_HOST=db etc. itself and depends_on db, so `docker compose run --rm cli php artisan test` works against real MariaDB with zero per-developer .env.testing edits - Add docker-resources/mariadb/init/01-create-test-db.sql to provision a dedicated invoiceplane_test database alongside the dev one on first boot - Fix db service: the named `database` volume was declared but never mounted, so all local dev/test data was lost on every container recreate - docker-resources/php-cli/Dockerfile: rebuild on Debian (php:8.4-cli) with the minimal proven extension set, matching the ip2-test-php:8.4 image this session used successfully throughout — see InvoicePlane#689 for a still-open false- failure issue found with a fresh cli image build, flagged in the docs - Update AGENTS.md/CLAUDE.md/README.md/.github/DOCKER.md/Makefile to point at the compose db/cli path instead of the SQLite instructions - Note throughout: use `php artisan test`, not raw vendor/bin/phpunit — the two were observed to behave differently for this app's Livewire form tests
…try (Phase 1) Pure-file report template storage — a template is a folder with manifest.json + bands.json on the report_templates disk, no database tables. Ships default invoice/quote templates in resources/ and a reports:sync-system command that copies them into system storage. Harvests the 17 report bricks from feature/145-report-builder into Modules\Core\Mason (per module convention), rebased onto a ReportBrick base class that adds band placement rules and config-schema introspection. Brick signatures updated for the current awcodes/mason API (nullable toHtml data, BrickAction-managed insert flow). Adds PageBreak and Spacer utility bricks for InvoicePlane#95. Security per PRD: slugs restricted to [a-z0-9-], company paths derived from tenant context only, bands validated on load (unknown bricks skipped, widths enum-checked, configs filtered per brick schema). Refs InvoicePlane#130 InvoicePlane#521 InvoicePlane#528 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…any panels (Phase 2) Custom Filament pages (no Eloquent resource): a template list page and a builder page with five stacked Mason canvases, one per ReportBand, each offering only the bricks allowed in that band. Shared base classes in Modules\Core\Filament\Pages\Reports; the admin panel edits system templates, the company panel shows system templates read-only and manages tenant-scoped clones. Actions: Clone (into the panel's own scope), Rename, Delete (shipped defaults protected), Preview, and an explicit "move to band" action that validates allowedBands() — cross-canvas dragging is deliberately not supported. MasonDocumentConverter round-trips bands.json entries to mason editor state, carrying block width in a reserved config key. Also fixes AbstractCompanyPanelTestCase::testLivewire, which imported the mason schema component instead of the Livewire test facade and chained withSession onto the wrong object; imports mason plugin CSS into all five panel themes. Refs InvoicePlane#130 InvoicePlane#519 InvoicePlane#523 InvoicePlane#525 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReportRenderer turns manifest + bands + entity data into the HTML document for the PDF driver: bands render in document order, block widths become percentage columns, a band with keep_together in the manifest's band_options gets page-break-inside: avoid, and the PageBreak/Spacer bricks provide manual pagination control (InvoicePlane#95). ReportDataMapper builds the data arrays the brick views consume from Invoice/Quote models (company, client, meta, items, totals, terms, summary, footer). PdfGenerationService resolves the template chain — document slug, then company default (existing invoices.template and companies.invoice_template/quote_template columns), then the system default — and renders via the domPDF driver. The stubbed "download pdf" table actions on invoices and quotes now stream a real PDF. Fixes two latent bugs the pipeline exposed: the domPDF driver had no Dompdf imports (fatal on first use) and remote fetching enabled (violates the security requirement — now off, images must be local); Company::communications() used the wrong morph name (communicable vs the table's communicationable). Golden-file HTML snapshots for the default invoice and quote templates pin rendering output; PDF byte tests run the real dompdf engine. Refs InvoicePlane#130 InvoicePlane#95 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a nullable "PDF Template" select to the invoice and quote forms, listing the disk templates for that document type (system defaults plus the current company's clones, clones shadowing same-slug system templates). The selection persists in the existing invoices.template / quotes.template columns; clearing it falls back to the company default (companies.invoice_template / quote_template) and then the shipped system default — no schema changes were needed, the columns already existed. Resolution-chain behavior is covered in PdfGenerationServiceTest; this adds the picker-level scenarios (options per type, clone shadowing, persistence, clearing). Closes InvoicePlane#411; Refs InvoicePlane#130 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Visual smoke-testing the generated PDF showed dompdf ignores float clearing, so a full-width brick after two half-width bricks rendered on top of them. Bands now chunk consecutive bricks into 12-grid rows rendered as tables, which dompdf lays out correctly. Page-break bricks are emitted at block level between row tables because dompdf also ignores page-break CSS inside table cells. Golden fixtures regenerated for the new markup. Refs InvoicePlane#130 InvoicePlane#95 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iver Adds a Browsershot driver behind the existing PDFFactory abstraction for installs that have Node + Chromium: select it with IP_PDF_DRIVER=Browsershot, point IP_BROWSERSHOT_CHROME_PATH at a Chromium binary (node/npm/no-sandbox overrides available). dompdf stays the zero-dependency default; wkhtmltopdf/Snappy was rejected because its upstream is archived with unpatched CVEs, and mPDF (the v1 engine) is a CSS sidegrade. Verified end-to-end against a real Chromium: the same invoice HTML renders the identical band layout on both engines — the table-row markup from the dompdf fix is engine-independent by design. Also pins dompdf/dompdf as a direct composer requirement: the merged driver had been relying on a transitive install, which this change's composer update silently removed — a latent packaging bug. And fixes the company-header logo guard (isset → !empty) so documents without a logo don't render a broken image icon on Chromium. Refs InvoicePlane#130 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…alculation The InvoiceItemFactory and QuoteItemFactory have an afterMaking hook that recalculates tax_total based on tax_rate_id. The PdfGenerationServiceTest was passing hard-coded tax values to the factory, but the afterMaking hook was overwriting them (defaulting to 0% tax when no tax_rate_id was set). Fixed by: 1. Creating a 21% TaxRate for each test 2. Creating items directly with Model::create() instead of factory(), bypassing the afterMaking recalculation hook entirely This ensures the golden-snapshot fixtures use the correct hard-coded values (tax=21.00, total=121.00) regardless of factory behavior. Claude-Session: https://claude.ai/code/session_0141XKe7z1156ezVWWobTFAH
…r name
Blade's {{ }} auto-escapes output, so an unescaped comparison only
fails when Faker happens to generate a company name with an
apostrophe or other HTML-special character. Confirmed by forcing
"O'Kon, Schneider and Wisozk" locally against real MariaDB: fails
without e(), passes with it. See InvoicePlane#687.
nielsdrost7
force-pushed
the
feature/130-report-builder
branch
from
July 24, 2026 11:16
877362d to
789c7e0
Compare
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
1 participant
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.
Built on the test-infra PR #607 — merge that first. Until it merges, this diff also shows #607's single commit; after it merges, only the Report Builder commits remain.
Implements the Report Builder (bands + bricks) per the v2 spec in #130 (epic #506, Track A) — Phases 1–4 in five commits, one per phase plus a rendering fix. Supersedes the stalled #532 (assets harvested, not merged).
What's in here
bcbbafbReportTemplateStorage(manifest.json + bands.json, zero DB tables), shipped default invoice/quote templates, idempotentreports:sync-system, 17 bricks harvested intoModules\Core\Mason+ new PageBreak/Spacer bricks0f6aa7b83abbe1ReportRenderer+ReportDataMapper+ realPdfGenerationService; wired the stubbed "download pdf" actions; golden-file HTML snapshots + real dompdf byte tests84d061ctemplatecolumns; resolution chain document → company default → system defaultacdb249297c649PDFFactory(IP_PDF_DRIVER=Browsershot+IP_BROWSERSHOT_CHROME_PATH); pinsdompdf/dompdfas a direct dependency (was only transitive — latent packaging bug); logo guard fix. Verified against a real Chromium: identical band layout on both enginesSecurity (hard requirements from the PRD)
No user-authored PHP/Blade anywhere; slugs restricted to
[a-z0-9-](path traversal impossible); company storage paths derived from tenant context only; bands.json validated on load (unknown bricks skipped, widths enum-checked, configs filtered against each brick's own schema); dompdf remote fetching disabled (was enabled — fixed here); all brick views escape output.Latent bugs fixed en route
domPDFdriver had noDompdfimports (fatal on first use) andsetIsRemoteEnabled(true)Company::communications()used the wrong morph name (communicablevs table'scommunicationable)AbstractCompanyPanelTestCase::testLivewire()imported a mason schema component instead of the Livewire test facadeTesting
Full suite green: 400 tests, 1,339 assertions, 0 failures (develop baseline: 307 tests, also green). New coverage: storage round-trip/isolation/traversal, brick registry + band rules, builder pages in both panels (load/save/move/clone/delete/read-only), renderer (toggles, keep-together, page breaks, widths), golden snapshots for
invoice/default+quote/default, PDF bytes via real dompdf, and the #411 resolution/picker scenarios. Plus an end-to-end smoke: app boot → migrate → sync → factory invoice → visually verified PDF.Follow-up issues (tracked, not resolved here)
All hanging items have tracked issues on the main repo (sub-issues of #130); plain references only — this fork PR must not auto-close them:
group_by, valid item keys, per-group totals for Group Footer bricks); renderer currently renders every band exactly onceband_options.<band>.keep_togetherbut only hand-edited manifests can set it (remaining builder-side slice of [Core]: PDF templates — page-break support #95)Independent tracks, unchanged: tabular reports (#145, Track B) and email variables (#363, Track C).
Summary by CodeRabbit
New Features
Bug Fixes