Skip to content

feat: add Report Builder#472

Closed
nielsdrost7 wants to merge 2 commits into
developfrom
feature/130-report-builder
Closed

feat: add Report Builder#472
nielsdrost7 wants to merge 2 commits into
developfrom
feature/130-report-builder

Conversation

@nielsdrost7

@nielsdrost7 nielsdrost7 commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features
    • Added comprehensive report template builder with visual drag-and-drop editor for designing invoice and document layouts
    • Introduced block-based system for composing reports with configurable sections (header, details, footer)
    • Added ability to create, edit, clone, and delete report templates
    • Enabled block customization with configurable display options for fields (name, date, amounts, etc.)
    • Added system templates with cloning support for quick template creation

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

🟠 Major comments (33)
Modules/Core/DTOs/BlockDTO.php-91-97 (1)

91-97: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent null dereference in clone creation path

On Line 91, getPosition() can return null, but Line 93-96 immediately dereference it. This can hard-fail at runtime when cloning partially initialized DTOs.

Proposed fix
 public static function clonedFrom(self $original, string $newId): self
 {
+    $originalPosition = $original->getPosition();
+    if ($originalPosition === null) {
+        throw new \LogicException('Cannot clone block without a position.');
+    }
+
     $dto = new self();
     $dto->setId($newId);
     $dto->setType($original->getType());
     $dto->setSlug($original->getSlug());
-
-    $originalPosition = $original->getPosition();
     $newPosition      = GridPositionDTO::create(
         $originalPosition->getX(),
         $originalPosition->getY(),
         $originalPosition->getWidth(),
         $originalPosition->getHeight()
     );
🤖 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/DTOs/BlockDTO.php` around lines 91 - 97, BlockDTO's clone path
dereferences $original->getPosition() without null-check, which can cause a
fatal error; update the clone/creation logic (reference: $original,
getPosition(), $originalPosition, GridPositionDTO::create) to first check if
$originalPosition is null and only call GridPositionDTO::create when it's
non-null, otherwise set the new position to null (or an appropriate default) so
no method is called on a null value.
Modules/Core/Services/MasonStorageAdapter.php-98-109 (1)

98-109: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Use the transformer instead of manually assembling BlockDTO in the service.

createBlockFromMasonBrick() manually maps every field into BlockDTO, which duplicates transformation logic and increases drift risk against canonical mapping code.

As per coding guidelines, “Services must not build DTOs manually; instead, they must use Transformers directly”.

🤖 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/MasonStorageAdapter.php` around lines 98 - 109, The
service currently assembles a BlockDTO manually in createBlockFromMasonBrick by
setting id, type, slug, position, config, label, isCloneable, dataSource,
isCloned and clonedFrom; replace this manual mapping with the canonical
Transformer for blocks (use the Block Transformer/transformer service used
elsewhere) so the DTO is created via the transformer rather than by hand.
Specifically, call the appropriate transformer method (e.g.,
BlockTransformer->fromMasonBrick(...) or the project's transformer service)
passing the mason brick payload and any contextual values (ensure dataSource is
provided—use getDataSourceForType($type) or let the transformer obtain it if
supported), and remove the manual new BlockDTO() assembly in
createBlockFromMasonBrick so all fields are populated by the transformer.

Source: Coding guidelines

Modules/Core/DTOs/GridPositionDTO.php-42-48 (1)

42-48: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce one invariant model for grid coordinates across constructor/setters.

create() rejects invalid geometry, but Lines 42-48 and Lines 108-134 still allow constructing invalid DTO state (width/height = 0, negative coordinates via setters). This makes behavior dependent on which construction path is used.

Suggested fix
 public function __construct()
 {
     $this->x      = 0;
     $this->y      = 0;
-    $this->width  = 0;
-    $this->height = 0;
+    $this->width  = 1;
+    $this->height = 1;
 }

 public function setX(int $x): self
 {
+    if ($x < 0) {
+        throw new InvalidArgumentException('x must be >= 0');
+    }
     $this->x = $x;
     return $this;
 }

 public function setY(int $y): self
 {
+    if ($y < 0) {
+        throw new InvalidArgumentException('y must be >= 0');
+    }
     $this->y = $y;
     return $this;
 }

 public function setWidth(int $width): self
 {
+    if ($width <= 0) {
+        throw new InvalidArgumentException('width must be > 0');
+    }
     $this->width = $width;
     return $this;
 }

 public function setHeight(int $height): self
 {
+    if ($height <= 0) {
+        throw new InvalidArgumentException('height must be > 0');
+    }
     $this->height = $height;
     return $this;
 }

Also applies to: 108-134

🤖 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/DTOs/GridPositionDTO.php` around lines 42 - 48, The DTO
currently allows invalid state via the no-arg constructor and setters; make the
invariant (x >= 0, y >= 0, width > 0, height > 0) consistent with create() by
adding the same validation and failing fast: update __construct(), setX(),
setY(), setWidth(), and setHeight() to enforce x/y non-negative and width/height
strictly positive (throw InvalidArgumentException or similar on violation), or
require parameters in __construct() and validate them there; ensure the
validation logic exactly mirrors the checks used in create().
Modules/Core/Services/ReportBlockService.php-61-64 (1)

61-64: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Split exception handling to match the project’s required throwable granularity.

The current handlers use broad catch blocks and do not separate Error, ErrorException, and Throwable as required by project standards.

As per coding guidelines, “Catch Error, ErrorException, and Throwable separately”.

Also applies to: 104-109, 122-135

🤖 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/ReportBlockService.php` around lines 61 - 64, Replace
the broad catch(Throwable $e) handlers in ReportBlockService (the blocks that
call DB::rollBack() and rethrow) with separate catches for Error,
ErrorException, and Throwable in that order; for each catch ensure
DB::rollBack() is invoked (if a transaction is active) and then rethrow the
caught exception (preserving the original variable name $e) so Error is handled
first, ErrorException next, and any remaining Throwable last; update all
occurrences mentioned around the methods handling transactions (the catch blocks
at the locations with DB::rollBack()) to follow this three-way catch pattern.

Source: Coding guidelines

Modules/Core/Services/GridSnapperService.php-17-20 (1)

17-20: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard gridSize at construction time.

Line 17 accepts non-positive values, which makes snap/validate semantics inconsistent for the whole service. Add a hard precondition ($gridSize >= 1).

Suggested fix
+use InvalidArgumentException;
 ...
 public function __construct(int $gridSize = 12)
 {
+    if ($gridSize < 1) {
+        throw new InvalidArgumentException('gridSize must be >= 1');
+    }
     $this->gridSize = $gridSize;
 }
🤖 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/GridSnapperService.php` around lines 17 - 20, The
constructor GridSnapperService::__construct currently accepts non-positive grid
sizes causing snap/validate to behave incorrectly; add a precondition that
$gridSize >= 1 and throw a clear exception (e.g., InvalidArgumentException) when
the check fails so invalid instances cannot be constructed, and ensure the
$gridSize property is only set after the validation to keep snap and validate
methods consistent.
Modules/Core/Services/MasonStorageAdapter.php-69-73 (1)

69-73: ⚠️ Potential issue | 🟠 Major

Handle JSON encoding failures explicitly in blocksToMason().

Line 69 returns json_encode(...) directly (with JSON_PRETTY_PRINT only); json_encode() can return false on encoding errors (e.g., invalid UTF-8), which violates the : string return type at runtime. Use JSON_THROW_ON_ERROR and catch JsonException.

🤖 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/MasonStorageAdapter.php` around lines 69 - 73,
blocksToMason() currently returns json_encode(...) directly which can return
false on failure and break the : string contract; change the call to
json_encode(..., JSON_PRETTY_PRINT|JSON_THROW_ON_ERROR) inside a try/catch,
catch \JsonException and handle it (e.g., throw a more specific exception or log
and rethrow) to ensure a string is always returned or a controlled exception is
raised; update MasonStorageAdapter::blocksToMason to use JSON_THROW_ON_ERROR and
catch \JsonException so encoding errors are handled explicitly.
Modules/Core/Services/ReportBlockService.php-121-127 (1)

121-127: ⚠️ Potential issue | 🟠 Major

Handle JSON encoding errors when persisting block configuration.
In Modules/Core/Services/ReportBlockService.php (saveBlockFields, lines 121-127), json_encode($config, JSON_PRETTY_PRINT) is called without JSON_THROW_ON_ERROR and its return value isn’t validated. If encoding fails, json_encode returns false, and Storage::disk('local')->put() may persist an invalid/empty payload. Use JSON_THROW_ON_ERROR (or explicitly check the return value) and throw/log on JsonException before writing.

🤖 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/ReportBlockService.php` around lines 121 - 127, In
saveBlockFields (ReportBlockService::saveBlockFields) ensure JSON encoding is
validated before calling Storage::disk('local')->put: replace the raw
json_encode($config, JSON_PRETTY_PRINT) usage with encoding that either uses
JSON_THROW_ON_ERROR or checks the return value, catch JsonException (or detect
false), log the encoding error with the existing logger keys ('path' and
'error') and throw a RuntimeException (preserving the original exception as
previous) instead of writing an invalid payload to storage; do this validation
and error handling before the Storage::put call so only valid JSON is persisted.
Modules/Core/Services/ReportTemplateService.php-353-381 (1)

353-381: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Avoid manual DTO building in service; route through Transformer.

This service constructs BlockDTO via setter chain directly. Move construction through BlockTransformer to keep a single mapping path.

As per coding guidelines, "Services must not build DTOs manually; instead, they must use Transformers directly."

🤖 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/ReportTemplateService.php` around lines 353 - 381, The
createSystemBlock method is manually instantiating and populating a BlockDTO
(BlockDTO and createSystemBlock), which violates the rule to build DTOs via the
BlockTransformer; instead inject or obtain the BlockTransformer instance and
replace the setter-chain construction with a single transform call on
BlockTransformer (e.g. provide the same fields: id, type, slug,
position/GridPositionDTO, config, label, isCloneable, dataSource, band,
isCloned, clonedFrom) so the BlockDTO is produced by BlockTransformer; remove
the direct new BlockDTO() and setter usage and ensure the produced DTO matches
the previous field values.

Source: Coding guidelines

Modules/Core/Database/Seeders/ReportBlocksSeeder.php-101-105 (1)

101-105: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard optional block config before mutation.

Line 103 reads $block['config'] unconditionally, but most seeded entries do not define config. Use a safe default before setting fields.

Suggested fix
-                $config           = $block['config'];
+                $config           = $block['config'] ?? [];
                 $config['fields'] = []; // Start with no fields as requested for drag/drop
                 Storage::disk('local')->put($path, json_encode($config, JSON_PRETTY_PRINT));
🤖 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/Database/Seeders/ReportBlocksSeeder.php` around lines 101 - 105,
Guard the optional block config before mutating it in ReportBlocksSeeder.php:
instead of reading $block['config'] unconditionally, initialize $config from the
block using a safe default (e.g. $config = isset($block['config']) ?
$block['config'] : [] or the null coalescing operator) and then set
$config['fields'] = [] before writing to storage; ensure you mutate the local
$config variable (not $block directly) so missing config keys don't trigger
notices.
Modules/Core/Services/ReportTemplateService.php-293-310 (1)

293-310: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use loaded config when constructing system blocks.

Line 293 fetches $config, but Line 306 hardcodes null. This drops persisted defaults and makes block config behavior inconsistent.

Suggested fix
-            $blocks[$dbBlock->block_type->value] = $this->createSystemBlock(
+            $blocks[$dbBlock->block_type->value] = $this->createSystemBlock(
                 'block_' . $dbBlock->block_type->value,
                 $dbBlock->block_type,
                 $dbBlock->slug,
                 0,
                 0,
                 $width,
                 4,
-                null,
+                is_array($config) ? $config : [],
                 $dbBlock->name,
                 $dbBlock->data_source->value,
                 $dbBlock->default_band->value
             );
🤖 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/ReportTemplateService.php` around lines 293 - 310, The
code calls getBlockConfig($dbBlock) into $config but then passes null for the
config when creating system blocks; update the createSystemBlock invocation in
ReportTemplateService to pass the previously loaded $config instead of null so
persisted defaults are preserved — locate the getBlockConfig($dbBlock) call and
the createSystemBlock(...) call and replace the null config argument with
$config (ensuring the parameter order for createSystemBlock remains correct).

Source: Linters/SAST tools

Modules/Core/Services/ReportTemplateService.php-289-291 (1)

289-291: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Scope getSystemBlocks() to system records only.

Current query includes any active block. That can expose tenant/custom blocks as “system” clone sources.

Suggested fix
-        $dbBlocks = ReportBlock::where('is_active', true)->get();
+        $dbBlocks = ReportBlock::where('is_active', true)
+            ->where('is_system', true)
+            ->get();
🤖 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/ReportTemplateService.php` around lines 289 - 291, The
current retrieval in getSystemBlocks uses ReportBlock::where('is_active',
true)->get() which returns any active block; restrict it to system-only records
by adding the system condition (e.g., ->where('is_system', true) or the
project’s canonical system indicator) to the query so $dbBlocks only contains
system blocks; update the query in getSystemBlocks (referencing $blocks and
$dbBlocks) to include that additional where clause and return only those system
records.
Modules/Core/Database/Seeders/ReportBlocksSeeder.php-78-93 (1)

78-93: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make seeding idempotent; random slugs currently duplicate system blocks.

Using Str::random(8) for every run creates new slug/filename, so rerunning the seeder inserts duplicate system records and extra config files.

Suggested fix
-            $baseSlug = Str::slug($block['name']);
-            $slug     = $baseSlug . '-' . Str::random(8);
-            $filename = $slug;
+            $baseSlug = Str::slug($block['name']);
+            $slug     = $baseSlug;
+            $filename = $baseSlug;

-            ReportBlock::create([
+            ReportBlock::query()->firstOrCreate([
+                'slug' => $slug,
+            ], [
                 'is_active'    => true,
                 'is_system'    => true,
                 'block_type'   => $block['block_type'],
                 'name'         => $block['name'],
                 'slug'         => $slug,
                 'filename'     => $filename,
                 'width'        => $block['width'],
                 'data_source'  => $block['data_source'],
                 'default_band' => $block['default_band'],
-            ]);
+            ]);
🤖 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/Database/Seeders/ReportBlocksSeeder.php` around lines 78 - 93,
The seeder currently uses Str::random(8) so each run creates new ReportBlock
rows; make it idempotent by locating or upserting the system block instead of
always creating one. In ReportBlocksSeeder, compute $baseSlug =
Str::slug($block['name']) and then use ReportBlock::firstOrCreate (or
ReportBlock::updateOrCreate) keyed on ['name' => $block['name'], 'block_type' =>
$block['block_type'], 'is_system' => true] to reuse existing records; when
creating for the first time set 'slug' and 'filename' from the deterministic
$baseSlug (or append a deterministic suffix if needed) and preserve any existing
slug/filename when the record already exists so rerunning the seeder won’t
insert duplicates.
Modules/Core/Services/ReportTemplateService.php-206-214 (1)

206-214: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent DB/file divergence on delete failure.

If file deletion fails, the model is still deleted at Line 213. That leaves orphaned template files with no DB owner.

Suggested fix
-        if ( ! $deleted) {
-            Log::warning('Failed to delete report template file', [
-                'company_id' => $template->company_id,
-                'slug'       => $template->slug,
-            ]);
-        }
-
-        $template->delete();
+        if ( ! $deleted) {
+            Log::warning('Failed to delete report template file', [
+                'company_id' => $template->company_id,
+                'slug'       => $template->slug,
+            ]);
+            throw new \RuntimeException('Template file deletion failed; aborting model delete.');
+        }
+
+        $template->delete();
🤖 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/ReportTemplateService.php` around lines 206 - 214, The
code currently calls $template->delete() regardless of the $deleted flag,
causing orphaned files when file removal fails; change the flow so the model is
only deleted when $deleted is truthy — replace the unconditional
$template->delete() call with a conditional that checks $deleted and if false
logs the failure (Log::warning already present) and aborts the delete operation
(return false or throw an exception) so DB and filesystem stay consistent;
ensure you reference and use the existing $deleted variable and
$template->delete() invocation in ReportTemplateService.php.
Modules/Core/Transformers/BlockTransformer.php-101-111 (1)

101-111: ⚠️ Potential issue | 🟠 Major

Handle JSON encoding failures explicitly in Modules/Core/Transformers/BlockTransformer.php::toJson().

json_encode() can return false, but toJson() declares a string return type, which can lead to a TypeError. Add JSON_THROW_ON_ERROR to make encoding failures raise JsonException instead.

Suggested fix
     public static function toJson(BlockDTO $dto, bool $pretty = true): string
     {
         $array = self::toArray($dto);
-        $flags = JSON_UNESCAPED_SLASHES;
+        $flags = JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR;

         if ($pretty) {
             $flags |= JSON_PRETTY_PRINT;
         }

         return json_encode($array, $flags);
     }
🤖 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/Transformers/BlockTransformer.php` around lines 101 - 111, The
toJson method in BlockTransformer::toJson currently calls json_encode which can
return false and violates the string return type; update the json_encode call to
include the JSON_THROW_ON_ERROR flag (combine it with JSON_UNESCAPED_SLASHES and
JSON_PRETTY_PRINT when applicable) so encoding failures throw a JsonException
instead of returning false, and ensure any callers handle or document the thrown
JsonException from BlockTransformer::toJson.
Modules/Core/Handlers/DetailItemTaxBlockHandler.php-33-47 (1)

33-47: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Root cause: new report block handlers render user-facing labels as hardcoded literals instead of translation keys.
Modules/Core/Handlers/DetailItemTaxBlockHandler.php and Modules/Core/Handlers/DetailItemsBlockHandler.php both need trans()-based labels to keep report output localized and consistent with InvoicePlane conventions.

As per coding guidelines, "Use trans() function for all translations, NOT __() - follow the trans() convention for InvoicePlane v2".

🤖 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/Handlers/DetailItemTaxBlockHandler.php` around lines 33 - 47,
Replace hardcoded user-facing labels in the report block handlers with trans()
calls: in DetailItemTaxBlockHandler (class/method that builds $html) change "Tax
Details", "Tax Name", "Rate", "Amount" to their translation keys wrapped with
trans() (e.g. trans('...')) and ensure the same conversion is applied in
DetailItemsBlockHandler for any literal column or header labels; use the
existing InvoicePlane translation keys/naming convention and do not use __(),
only trans().

Source: Coding guidelines

Modules/Core/Tests/Unit/ReportBlockWidthTest.php-30-30 (1)

30-30: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove markTestIncomplete() from finished assertions so tests actually enforce behavior.

Leaving all test paths incomplete prevents these checks from guarding regressions.

As per coding guidelines, "Tests must be meaningful - avoid simple 'ok' checks; validate actual behavior and data".

Also applies to: 46-46, 62-62, 78-78, 102-102, 122-122

🤖 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/ReportBlockWidthTest.php` at line 30, Remove the
placeholder markTestIncomplete() calls from the ReportBlockWidthTest tests
(class ReportBlockWidthTest) so the tests actually run; locate each occurrence
and either delete the markTestIncomplete() invocation or replace it with
concrete assertions that validate the expected behavior/data for that test case
(assertEquals/assertTrue/etc. targeting the method under test). Ensure each test
method contains meaningful assertions that exercise the code paths previously
marked incomplete and run the test suite to confirm failures surface when
behavior regresses.

Source: Coding guidelines

Modules/Core/Handlers/FooterQrCodeBlockHandler.php-35-53 (1)

35-53: ⚠️ Potential issue | 🟠 Major

Avoid leaking full invoice URLs to a third-party QR service (api.qrserver.com).
Modules/Core/Handlers/FooterQrCodeBlockHandler.php builds $qrData as url('/invoices/view/' . $invoice->url_key) and sends it to https://api.qrserver.com/v1/create-qr-code/...&data= (URL-encoded), so the third party receives the invoice report identifier. Generate QR codes locally (or via a self-hosted/internal endpoint) 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/Handlers/FooterQrCodeBlockHandler.php` around lines 35 - 53,
FooterQrCodeBlockHandler currently sends the full invoice URL to
api.qrserver.com by building $qrData in generateQrData() and embedding it into
the external img src; change this to avoid leaking invoice URLs by either (A)
returning only the invoice identifier (e.g., $invoice->url_key) from
generateQrData() and updating the img src assembly to point to an internal
endpoint that serves QR images (e.g., /internal/qr/invoice/{key}), or (B)
generate the QR image locally on the server (use a PHP QR library inside
FooterQrCodeBlockHandler and output a data URI or save a local path) instead of
calling https://api.qrserver.com; update the code that constructs the <img
src="..."> (the string concatenation building the QR image tag) to use the
internal endpoint or local data URI and ensure include_url behavior still
HTML-escapes displayed values.
Modules/Core/Handlers/FooterQrCodeBlockHandler.php-25-36 (1)

25-36: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Sanitize and bound size before interpolating into HTML attributes.

$size is injected directly into src, width, and height. If config is tampered with, this becomes an attribute/script injection path.

Suggested fix
-        $size   = $config['size'] ?? 100;
+        $size = (int) ($config['size'] ?? 100);
+        $size = max(32, min(512, $size));
🤖 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/Handlers/FooterQrCodeBlockHandler.php` around lines 25 - 36, The
$size value used in FooterQrCodeBlockHandler (in the block that calls
$this->generateQrData($invoice) and builds the <img> src/width/height) is taken
directly from $config and injected into attributes; validate and bound it before
interpolation by casting to an integer (e.g., $size = (int)($config['size'] ??
100)), enforce safe min/max limits (e.g., clamp to a sensible range like 50–1000
or your app policy) and use the sanitized integer for src/width/height; also
ensure you HTML-escape any dynamic strings when concatenating the tag to avoid
injection.
Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php-57-57 (1)

57-57: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove markTestIncomplete() from committed test cases.

All test methods are effectively disabled despite having assertions, so regressions in block-field persistence won’t be gated by CI.

As per coding guidelines, “Tests must be meaningful - avoid simple 'ok' checks; validate actual behavior and data.”

Also applies to: 89-89, 111-111, 137-137, 167-167, 192-192, 225-225, 261-261

🤖 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/ReportBlockServiceFieldsTest.php` at line 57, In
ReportBlockServiceFieldsTest remove all $this->markTestIncomplete(...) calls
(the one in ReportBlockServiceFieldsTest and the other occurrences referenced at
lines 89, 111, 137, 167, 192, 225, 261) so the test methods actually execute
their assertions; keep the existing assertions and setup intact, delete the
markTestIncomplete statements, then run the test suite (phpunit) to ensure the
tests now fail/pass appropriately and adjust any test expectations if needed.

Source: Coding guidelines

Modules/Core/Handlers/FooterTotalsBlockHandler.php-35-59 (1)

35-59: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Root cause: untranslated hardcoded UI strings in report block HTML renderers.
FooterTotalsBlockHandler.php, HeaderClientBlockHandler.php, HeaderCompanyBlockHandler.php, and HeaderInvoiceMetaBlockHandler.php each emit user-facing labels as raw literals. Standardize these labels on trans('...') keys in all four files to keep report rendering localization-compliant.

As per coding guidelines, “Use trans() function for all translations, NOT __() - follow the trans() convention for InvoicePlane v2.”

🤖 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/Handlers/FooterTotalsBlockHandler.php` around lines 35 - 59, The
footer totals block emits hardcoded user-facing labels (e.g. "Subtotal",
"Discount", "Tax", "Total", "Paid", "Balance Due") in
FooterTotalsBlockHandler::render (and similarly in HeaderClientBlockHandler,
HeaderCompanyBlockHandler, HeaderInvoiceMetaBlockHandler); replace each literal
label string with the trans('...') call using appropriate translation keys (e.g.
trans('invoice.subtotal') etc.) while leaving numeric formatting
(formatCurrency) intact and ensure you use trans() (not __()); update the
corresponding label occurrences in those four handler files to restore
localization compliance.

Source: Coding guidelines

Modules/Core/Handlers/FooterTotalsBlockHandler.php-39-59 (1)

39-59: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Escape formatted currency output before concatenating into HTML.

These rows inject dynamic values into HTML directly. currency_code is data-backed and should be escaped at render time to prevent stored-XSS paths in report previews/exports.

Proposed hardening
- $html .= '<tr><td>Subtotal:</td><td>' . $this->formatCurrency($invoice->subtotal ?? 0, $invoice->currency_code) . '</td></tr>';
+ $html .= '<tr><td>' . trans('ip.subtotal') . ':</td><td>' . e($this->formatCurrency((float) ($invoice->subtotal ?? 0), $invoice->currency_code)) . '</td></tr>';
🤖 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/Handlers/FooterTotalsBlockHandler.php` around lines 39 - 59, The
HTML rows in FooterTotalsBlockHandler are concatenating unescaped dynamic values
(e.g., $invoice->subtotal, $invoice->currency_code, $invoice->discount,
$invoice->tax, $invoice->total, $invoice->paid, $invoice->balance) returned from
formatCurrency directly into $html, creating a stored-XSS risk; update the code
that builds these rows (the block in FooterTotalsBlockHandler that appends to
$html) to escape user-controlled values before concatenation—specifically wrap
$invoice->currency_code and the output of $this->formatCurrency(...) with a
proper HTML-escaping function (e.g., htmlspecialchars(..., ENT_QUOTES, 'UTF-8'))
so all interpolated strings are safely encoded before being added to $html.
Modules/Core/Services/BlockFactory.php-35-44 (1)

35-44: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Unify canonical block type keys across the backend pipeline.

make() resolves keys like company_header / invoice_items, while the Mason conversion test fixtures use types like header_company / detail_items. This key drift is a contract break that can trigger InvalidArgumentException at runtime when converted blocks are resolved through the factory.

Suggested direction
- return match ($type) {
-     'company_header'      => app(HeaderCompanyBlockHandler::class),
-     'invoice_items'       => app(DetailItemsBlockHandler::class),
+ return match ($type) {
+     ReportBlockType::HEADER_COMPANY->value => app(HeaderCompanyBlockHandler::class),
+     ReportBlockType::DETAIL_ITEMS->value   => app(DetailItemsBlockHandler::class),
      ...
- 'type' => 'company_header',
+ 'type' => ReportBlockType::HEADER_COMPANY->value,

Also applies to: 57-107

🤖 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/BlockFactory.php` around lines 35 - 44, The
BlockFactory's match on $type uses keys like 'company_header' and
'invoice_items' which mismatch the Mason fixture canonical keys
('header_company', 'detail_items'), causing InvalidArgumentException; update the
factory (the make method in BlockFactory.php where the return match is defined)
to either standardize to the canonical fixture names (e.g., 'header_company',
'header_client', 'header_invoice_meta', 'detail_items', 'detail_item_tax',
'footer_totals', 'footer_notes', 'footer_qr_code') or add alias entries so each
handler (HeaderCompanyBlockHandler, HeaderClientBlockHandler,
HeaderInvoiceMetaBlockHandler, DetailItemsBlockHandler,
DetailItemTaxBlockHandler, FooterTotalsBlockHandler, FooterNotesBlockHandler,
FooterQrCodeBlockHandler) is returned for both legacy and canonical keys (e.g.,
map both 'company_header' and 'header_company' to HeaderCompanyBlockHandler) to
ensure consistent resolution across the pipeline.
Modules/Core/Repositories/ReportTemplateFileRepository.php-138-142 (1)

138-142: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

all() currently returns Mason editor files as report template slugs.

This method returns every filename stem under the company directory, so mason_{slug}.json files from Mason storage will be surfaced as template slugs.

Suggested fix
- $files = Storage::disk('report_templates')->files($directory);
+ $files = array_filter(
+     Storage::disk('report_templates')->files($directory),
+     static fn (string $file): bool => ! str_starts_with(pathinfo($file, PATHINFO_FILENAME), 'mason_')
+ );
🤖 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/Repositories/ReportTemplateFileRepository.php` around lines 138
- 142, The all() method is returning Mason files like "mason_{slug}.json" as
template slugs; update ReportTemplateFileRepository::all to filter out files
whose basename starts with "mason_" (or otherwise match the Mason filename
pattern) before mapping to PATHINFO_FILENAME so only real report template files
are returned; locate the Storage::disk('report_templates')->files($directory)
call and apply an array_filter (or Collection filter) to exclude files with
basenames beginning with "mason_" prior to the existing pathinfo mapping.
Modules/Core/Repositories/ReportTemplateFileRepository.php-174-176 (1)

174-176: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard templateSlug before composing filesystem paths.

$templateSlug is interpolated directly into storage paths. Without an allowlist, traversal payloads can target unintended paths. Apply strict slug validation here, and reuse the same validation in MasonTemplateStorage::getTemplatePath().

Suggested fix
 protected function getTemplatePath(int $companyId, string $templateSlug): string
 {
+    if (! preg_match('/^[a-z0-9_-]+$/i', $templateSlug)) {
+        throw new \InvalidArgumentException('Invalid template slug.');
+    }
     return "{$companyId}/{$templateSlug}.json";
 }
🤖 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/Repositories/ReportTemplateFileRepository.php` around lines 174
- 176, Validate and sanitize templateSlug before interpolating into filesystem
paths: in ReportTemplateFileRepository::getTemplatePath and mirror the same
logic in MasonTemplateStorage::getTemplatePath, enforce an allowlist (e.g.,
strict regex like only lowercase letters, numbers, hyphen/underscore) or
normalize/escape invalid characters, and throw or return a controlled error when
validation fails so path traversal characters (../, /, null bytes, etc.) cannot
be used to craft arbitrary file paths.
Modules/Core/Filament/Admin/Resources/ReportTemplates/Tables/ReportTemplatesTable.php-80-83 (1)

80-83: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Edit action drops most submitted form fields.

The custom edit handler only sends blocks to the service, so user edits to template fields from the modal are not persisted. Either map and persist all editable inputs or remove those editable fields from this action’s form.

🤖 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/Admin/Resources/ReportTemplates/Tables/ReportTemplatesTable.php`
around lines 80 - 83, The edit action in ReportTemplatesTable (the closure
passed to ->action) only extracts $blocks from $data and calls
ReportTemplateService::updateTemplate($record, $blocks), which drops other
editable fields from the modal; update the handler to collect and persist all
submitted fields (e.g., merge other editable inputs from $data into $record or
pass the full $data to the service) so title/description/etc. are saved, or
remove those fields from the modal; specifically, modify the closure that
currently uses $blocks to instead pass the full payload (or call
$record->fill(...) and save) and update ReportTemplateService::updateTemplate
signature/implementation if necessary to accept and persist the full $data
rather than only $blocks.
Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php-94-98 (1)

94-98: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Block lookup can update the wrong record.

Resolving by id OR slug OR block_type and then taking first() is ambiguous. If multiple blocks share a block_type, the update path can target an unrelated row.

Prefer resolving by a unique identifier only (id or slug), and reject when resolution is ambiguous.

Also applies to: 171-175, 244-248

🤖 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/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php`
around lines 94 - 98, The current lookup for ReportBlock using
ReportBlock::query()->where('id', $blockSlug)->orWhere('slug',
$blockSlug)->orWhere('block_type', $blockSlug)->first() is ambiguous and can
update the wrong row; change the logic in the ReportBuilder methods that set
$block (the occurrences near the current three diffs) to resolve by a single
unique identifier only: if $blockSlug is an integer/id use where('id',
$blockSlug), otherwise use where('slug', $blockSlug'); run get() instead of
first() and reject with an error (or throw/return early) if the query returns
zero or more than one record to avoid ambiguous updates; remove use of
block_type entirely from the lookup. Ensure the same change is applied to the
other two similar lookups so all updates use unique resolution and explicit
ambiguity checks.
Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php-32-39 (1)

32-39: ⚠️ Potential issue | 🟠 Major

Declare and initialize $blocks as component state.

ReportBuilder reads/writes $this->blocks[...] in multiple handlers, but $blocks isn’t declared as a public component property—so Livewire won’t be able to reliably hydrate/persist that state across requests (see ReportBuilder.php around lines 295-419).

Suggested fix
 class ReportBuilder extends Page
 {
     use InteractsWithActions;
     use InteractsWithSchemas;

     public ReportTemplate $record;
+    public array $blocks = [];
 
     public string $masonContent = '';
🤖 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/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php`
around lines 32 - 39, ReportBuilder reads and mutates $this->blocks in multiple
handlers but never declares it as component state; declare and initialize a
public property named $blocks (e.g., public array $blocks = []) on the
ReportBuilder class so Livewire can hydrate/persist it across requests, and
ensure any methods that reference $this->blocks (the handlers around the block
manipulation logic) use this property rather than relying on local variables.
Modules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php-17-20 (1)

17-20: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replace all user-facing strings with trans() in this view.

This template still contains hardcoded English strings and @lang(...); the project convention requires trans() for user-facing text.

As per coding guidelines, “All user-facing text must use trans() …” and “Use trans() function for all translations, NOT __().”

Also applies to: 29-29, 55-55, 70-70, 82-82

🤖 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/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php`
around lines 17 - 20, The view contains hardcoded user-facing strings and an
`@lang` call that must be converted to use the trans() function; replace
`@lang`('ip.available_fields') with trans('ip.available_fields') and wrap the
literal string "Drag fields to the canvas to configure block layout" (and the
other occurrences at the mentioned locations) with appropriate trans('...')
keys, adding those keys to the locale files if missing; search for the other
hardcoded strings referenced (lines noted in the comment) and uniformly replace
any __(), `@lang` or plain text usage with trans('your.translation.key') so all
user-facing text in the template uses trans().

Source: Coding guidelines

Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/CreateReportTemplate.php-46-53 (1)

46-53: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Constrain company resolution to the authenticated user’s scope.

The current lookup accepts any current_company_id present in session without verifying ownership, and can still pass null into creation flow. Resolve via the authenticated user’s companies relation and fail fast when no company is available.

Suggested fix
-        $company = Company::find(session('current_company_id'));
-        if ( ! $company) {
-            $company = auth()->user()->companies()->first();
-        }
+        $user = auth()->user();
+        $company = $user?->companies()->find(session('current_company_id'))
+            ?? $user?->companies()->first();
+
+        if (! $company) {
+            abort(403, 'No accessible company context for template creation.');
+        }
🤖 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/Admin/Resources/ReportTemplates/Pages/CreateReportTemplate.php`
around lines 46 - 53, The company lookup currently trusts
session('current_company_id') and may return a Company the authenticated user
doesn't own or null; update the logic in the CreateReportTemplate flow to
resolve $company from the authenticated user's companies relation (e.g. use
auth()->user()->companies()->where('id',
session('current_company_id'))->first()) and if that returns null fall back to
auth()->user()->companies()->first(); if still null fail fast (throw or abort)
before calling ReportTemplateService::createTemplate so createTemplate always
receives a valid company owned by the user.
Modules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php-114-119 (1)

114-119: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Scope hidden-input updates to this component instance.

Using document.querySelector('input[name="data[fields_canvas]"]') is global and can update the wrong form field if multiple canvases are rendered. Query from component root (this.$root) or use an x-ref target.

Suggested fix
-                const hiddenInput = document.querySelector('input[name="data[fields_canvas]"]');
+                const hiddenInput = this.$root.querySelector('input[name="data[fields_canvas]"]');
🤖 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/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php`
around lines 114 - 119, The updateFormField method currently uses
document.querySelector('input[name="data[fields_canvas]"]') which is global and
may target the wrong form; change updateFormField in this component to scope the
query to the component root (e.g. this.$root.querySelector(...)) or, better,
reference an x-ref on the hidden input and read it via this.$refs or
this.$root.querySelector('[x-ref="fieldsCanvasHidden"]'), then set
hiddenInput.value = JSON.stringify(this.canvasFields) and dispatch the change
event so only this instance's hidden field is updated; update any template to
add the x-ref attribute (e.g. x-ref="fieldsCanvasHidden") to the corresponding
input.
Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php-60-260 (1)

60-260: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Shared root cause: regression coverage is disabled via markTestIncomplete() across builder feature tests.

Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php, Modules/Core/Tests/Feature/ReportBuilderBlockWidthTest.php, and Modules/Core/Tests/Feature/ReportBuilderFieldCanvasIntegrationTest.php all include incomplete markers in otherwise asserted tests, which suppresses meaningful enforcement of report-builder behavior.

As per coding guidelines, "Tests must be meaningful - avoid simple 'ok' checks; validate actual behavior and data."

🤖 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/ReportBuilderBlockEditTest.php` around lines 60 -
260, Multiple tests in ReportBuilderBlockEditTest (e.g.,
it_populates_form_with_block_data, it_converts_width_enum_to_value_for_form,
it_provides_default_values_when_block_not_found,
it_logs_block_data_for_debugging, it_handles_all_block_types_correctly,
it_preserves_config_array_when_editing, it_uses_slug_for_lookup_when_available)
are marked incomplete with markTestIncomplete(), which disables their
assertions; remove the markTestIncomplete() calls in these test methods (or
replace them with meaningful final assertions if any checks are still missing)
so the existing assertions run, keep the arranged fixtures and assertions in
each method, and ensure any enum conversion stub (the BackedEnum check in
it_converts_width_enum_to_value_for_form) remains to produce the expected value;
run the test suite after edits to confirm all tests now fail/pass appropriately.

Source: Coding guidelines

Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ListReportTemplates.php-20-27 (1)

20-27: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard missing company context before calling createTemplate().

If session and user fallback both fail, $company can be null and this call path can crash at runtime.

💡 Suggested fix
                 ->action(function (array $data) {
                     $company = Company::query()->find(session('current_company_id'));
                     if ( ! $company) {
                         $company = auth()->user()->companies()->first();
                     }
+                    if ( ! $company) {
+                        throw new \RuntimeException(trans('Company context is required.'));
+                    }

-                    $template = app(ReportTemplateService::class)->createTemplate(
+                    app(ReportTemplateService::class)->createTemplate(
                         $company,
                         $data['name'],
                         $data['template_type'],
                         []
                     );
                 })

As per coding guidelines, "Use early returns for readability in PHP methods" and "Use trans() function for all translations, NOT __()."

🤖 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/Admin/Resources/ReportTemplates/Pages/ListReportTemplates.php`
around lines 20 - 27, The code may call ReportTemplateService::createTemplate
with a null $company; modify the ListReportTemplates.php flow to guard and
early-return when no company is found: after resolving $company via
Company::query()->find(session('current_company_id')) and
auth()->user()->companies()->first(), if $company is null, add an early return
(or redirect) with a user-facing message using trans() for translations (do not
use __()), and only call
app(ReportTemplateService::class)->createTemplate($company, $data['name'], ...)
when $company is present.

Source: Coding guidelines

Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/EditReportTemplate.php-15-36 (1)

15-36: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Wrap this custom save lifecycle in a DB transaction to avoid partial writes.

handleRecordUpdate() persists data before later steps complete. If a later step throws, the record remains updated while the request still fails, which creates a partial-save state.

💡 Suggested fix
+use Illuminate\Support\Facades\DB;
@@
     public function save(bool $shouldRedirect = true, bool $shouldSendSavedNotification = true): void
     {
         $this->authorizeAccess();
-
-        $this->callHook('beforeValidate');
-        $data = $this->form->getState();
-        $this->callHook('afterValidate');
-
-        $data = $this->mutateFormDataBeforeSave($data);
-        $this->callHook('beforeSave');
-
-        $this->record = $this->handleRecordUpdate($this->getRecord(), $data);
-
-        $this->callHook('afterSave');
-
-        if ($shouldSendSavedNotification) {
-            $this->getSavedNotification()?->send();
-        }
-
-        if ($shouldRedirect) {
-            $this->redirect($this->getRedirectUrl());
-        }
+        DB::transaction(function () use ($shouldRedirect, $shouldSendSavedNotification): void {
+            $this->callHook('beforeValidate');
+            $data = $this->form->getState();
+            $this->callHook('afterValidate');
+
+            $data = $this->mutateFormDataBeforeSave($data);
+            $this->callHook('beforeSave');
+
+            $this->record = $this->handleRecordUpdate($this->getRecord(), $data);
+            $this->callHook('afterSave');
+
+            if ($shouldSendSavedNotification) {
+                $this->getSavedNotification()?->send();
+            }
+
+            if ($shouldRedirect) {
+                $this->redirect($this->getRedirectUrl());
+            }
+        });
     }
🤖 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/Admin/Resources/ReportTemplates/Pages/EditReportTemplate.php`
around lines 15 - 36, The save() flow can produce partial writes because
handleRecordUpdate() persists before later steps complete; wrap the sequence
that modifies the DB (from mutateFormDataBeforeSave() / handleRecordUpdate() and
the surrounding hooks) in a database transaction (using
DB::beginTransaction()/commit()/rollBack() or DB::transaction) so any exception
rolls back the update; ensure hooks that must run inside the transaction
(beforeSave/afterSave) execute within it and move actions that should only run
after a successful commit (getSavedNotification()->send(), redirect()) to after
the transaction completes, rethrowing exceptions so normal error handling
applies.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 50b0dabc-ef10-4d67-94f8-78d221344809

📥 Commits

Reviewing files that changed from the base of the PR and between 0d38ec5 and 4b22e48.

📒 Files selected for processing (64)
  • Modules/Core/DTOs/BlockDTO.php
  • Modules/Core/DTOs/GridPositionDTO.php
  • Modules/Core/Database/Factories/ReportBlockFactory.php
  • Modules/Core/Database/Factories/ReportTemplateFactory.php
  • Modules/Core/Database/Migrations/2025_10_26_create_report_templates_table.php
  • Modules/Core/Database/Migrations/2026_01_01_184544_create_report_blocks_table.php
  • Modules/Core/Database/Seeders/ReportBlocksSeeder.php
  • Modules/Core/Enums/ReportBlockType.php
  • Modules/Core/Enums/ReportBlockWidth.php
  • Modules/Core/Enums/ReportTemplateType.php
  • Modules/Core/Filament/Admin/Resources/ReportBlocks/Pages/EditReportBlock.php
  • Modules/Core/Filament/Admin/Resources/ReportBlocks/Pages/ListReportBlocks.php
  • Modules/Core/Filament/Admin/Resources/ReportBlocks/ReportBlockResource.php
  • Modules/Core/Filament/Admin/Resources/ReportBlocks/Schemas/ReportBlockForm.php
  • Modules/Core/Filament/Admin/Resources/ReportBlocks/Tables/ReportBlocksTable.php
  • Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/CreateReportTemplate.php
  • Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/EditReportTemplate.php
  • Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ListReportTemplates.php
  • Modules/Core/Filament/Admin/Resources/ReportTemplates/Pages/ReportBuilder.php
  • Modules/Core/Filament/Admin/Resources/ReportTemplates/ReportTemplateResource.php
  • Modules/Core/Filament/Admin/Resources/ReportTemplates/Schemas/ReportTemplateForm.php
  • Modules/Core/Filament/Admin/Resources/ReportTemplates/Tables/ReportTemplatesTable.php
  • Modules/Core/Handlers/DetailItemTaxBlockHandler.php
  • Modules/Core/Handlers/DetailItemsBlockHandler.php
  • Modules/Core/Handlers/FooterNotesBlockHandler.php
  • Modules/Core/Handlers/FooterQrCodeBlockHandler.php
  • Modules/Core/Handlers/FooterTotalsBlockHandler.php
  • Modules/Core/Handlers/HeaderClientBlockHandler.php
  • Modules/Core/Handlers/HeaderCompanyBlockHandler.php
  • Modules/Core/Handlers/HeaderInvoiceMetaBlockHandler.php
  • Modules/Core/Interfaces/BlockHandlerInterface.php
  • Modules/Core/Models/ReportBlock.php
  • Modules/Core/Models/ReportTemplate.php
  • Modules/Core/Providers/AdminPanelProvider.php
  • Modules/Core/Repositories/ReportTemplateFileRepository.php
  • Modules/Core/Services/BlockFactory.php
  • Modules/Core/Services/GridSnapperService.php
  • Modules/Core/Services/MasonStorageAdapter.php
  • Modules/Core/Services/MasonTemplateStorage.php
  • Modules/Core/Services/ReportBlockService.php
  • Modules/Core/Services/ReportTemplateService.php
  • Modules/Core/Tests/Feature/BlockCloningTest.php
  • Modules/Core/Tests/Feature/CreateReportTemplateTest.php
  • Modules/Core/Tests/Feature/GridSnapperTest.php
  • Modules/Core/Tests/Feature/ReportBuilderBlockEditTest.php
  • Modules/Core/Tests/Feature/ReportBuilderBlockWidthTest.php
  • Modules/Core/Tests/Feature/ReportBuilderFieldCanvasIntegrationTest.php
  • Modules/Core/Tests/Feature/ReportBuilderMasonIntegrationTest.php
  • Modules/Core/Tests/Feature/UpdateReportTemplateTest.php
  • Modules/Core/Tests/Unit/BlockDTOTest.php
  • Modules/Core/Tests/Unit/BlockFactoryTest.php
  • Modules/Core/Tests/Unit/BlockTransformerTest.php
  • Modules/Core/Tests/Unit/GridPositionDTOTest.php
  • Modules/Core/Tests/Unit/GridSnapperServiceTest.php
  • Modules/Core/Tests/Unit/MasonBricksTest.php
  • Modules/Core/Tests/Unit/MasonStorageAdapterTest.php
  • Modules/Core/Tests/Unit/ReportBlockServiceFieldsTest.php
  • Modules/Core/Tests/Unit/ReportBlockWidthTest.php
  • Modules/Core/Tests/Unit/ReportBricksCollectionTest.php
  • Modules/Core/Tests/Unit/ReportTemplateFileRepositoryTest.php
  • Modules/Core/Tests/Unit/ReportTemplateServiceTest.php
  • Modules/Core/Tests/Unit/ReportTemplateTest.php
  • Modules/Core/Transformers/BlockTransformer.php
  • Modules/Core/resources/views/filament/admin/resources/report-blocks/fields-canvas.blade.php

Comment thread Modules/Core/Tests/Unit/ReportBlockWidthTest.php Outdated
- ReportTemplate and ReportBlock models, migrations, factories, seeders
- Filament admin resources: ReportTemplateResource, ReportBlockResource
- Block handlers (Header, Footer, Detail, QR code blocks)
- Services: ReportTemplateService, ReportBlockService, BlockFactory
- Mason storage adapter and template storage
- GridSnapperService and GridPositionDTO
- Block transformer and DTOs
- Report Builder page and blade templates
- Mason bricks collection (DetailItems, FooterTotals, HeaderClient, etc.)
- Full feature and unit test suite
- AdminPanelProvider: Report Builder navigation group and resources

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@nielsdrost7 nielsdrost7 force-pushed the feature/130-report-builder branch from 4b22e48 to 46f915b Compare June 12, 2026 21:48
@InvoicePlane InvoicePlane deleted a comment from coderabbitai Bot Jun 13, 2026
@nielsdrost7

Copy link
Copy Markdown
Collaborator Author

@coderabbitai autofix

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 1 file(s) based on 1 unresolved review comment.

Files modified:

  • Modules/Core/Tests/Unit/ReportBlockWidthTest.php

Commit: 2b20c838b752a4078a9912797e76140020ed4503

The changes have been pushed to the feature/130-report-builder branch.

Time taken: 1m 28s

Fixed 1 file(s) based on 1 unresolved review comment.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
@InvoicePlane InvoicePlane deleted a comment from coderabbitai Bot Jun 13, 2026
@nielsdrost7 nielsdrost7 deleted the feature/130-report-builder branch June 13, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant