[IP-145]: tabular reports — five company-panel report pages#609
Open
nielsdrost7 wants to merge 8 commits into
Open
[IP-145]: tabular reports — five company-panel report pages#609nielsdrost7 wants to merge 8 commits into
nielsdrost7 wants to merge 8 commits into
Conversation
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
Contributor
|
Warning Review limit reached
Next review available in: 57 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 selected for processing (34)
📝 WalkthroughWalkthroughIntroduces five company-scoped Filament reports with shared date/client filters, tabular views, summaries, CSV export, navigation registration, translations, and feature tests covering filtering, aggregation, scoping, rendering, and export. ChangesCompany tabular reports
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CompanyUser
participant FilamentReportPage
participant EloquentQuery
participant CsvDownload
CompanyUser->>FilamentReportPage: Select date and client filters
FilamentReportPage->>EloquentQuery: Execute filtered report query
EloquentQuery-->>FilamentReportPage: Return report rows
CompanyUser->>FilamentReportPage: Trigger CSV export
FilamentReportPage->>CsvDownload: Stream headers and rows
CsvDownload-->>CompanyUser: Download CSV
Possibly related issues
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Modules/Core/Filament/Company/Pages/Reports/BaseTabularReportPage.php (1)
111-118: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider using
pluckinstead ofmapfor performance.When generating key-value options from the database, using
pluckdirectly on the query builder is more efficient than fetching full models and mapping them in memory.♻️ Proposed fix
public function getClientOptions(): array { return Relation::query() ->orderBy('company_name') - ->get(['id', 'company_name']) - ->map(fn (Relation $relation): array => ['id' => $relation->id, 'name' => $relation->company_name]) + ->pluck('company_name', 'id') + ->map(fn (string $name, int $id): array => ['id' => $id, 'name' => $name]) + ->values() ->all(); }🤖 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/Company/Pages/Reports/BaseTabularReportPage.php` around lines 111 - 118, Update getClientOptions to use the query builder’s pluck operation instead of fetching Relation models with get and transforming them via map. Preserve the company_name ordering and return the same id-to-name option structure expected by callers.Modules/Core/Filament/Company/Pages/Reports/InvoicedByClientReport.php (1)
33-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid fetching all records into memory for the summary.
Calling
->get()on the full query loads all matching records into memory, which can lead to memory exhaustion and slow performance for companies with many clients. It is more efficient to calculate the summary values directly at the database level.♻️ Proposed fix
public function summaryLine(): string { - $rows = $this->reportQuery()->get(); + $query = $this->reportQuery(); + + $totalQuery = \Modules\Invoices\Models\Invoice::query() + ->whereBetween('invoiced_at', $this->dateRange()) + ->when($this->clientId, fn ($q) => $q->where('customer_id', $this->clientId)); return trans('ip.report_summary_invoiced_by_client', [ - 'clients' => $rows->count(), - 'total' => $this->money($rows->sum('invoiced_total')), + 'clients' => $query->count(), + 'total' => $this->money($totalQuery->sum('invoice_total')), ]); }🤖 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/Company/Pages/Reports/InvoicedByClientReport.php` around lines 33 - 41, Update summaryLine() to compute the client count and invoiced_total sum through database-level aggregate queries instead of calling get() and materializing all rows. Preserve the existing translation keys and money formatting, using the reportQuery() builder while ensuring the count and sum operate on the full matching dataset.Modules/Core/Filament/Company/Pages/Reports/InvoicesPerClientReport.php (1)
33-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid fetching all records into memory for the summary.
Loading all rows into memory via
->get()just to compute the total invoice count is inefficient and can cause memory issues for large datasets. Compute the totals at the database level instead.♻️ Proposed fix
public function summaryLine(): string { - $rows = $this->reportQuery()->get(); + $query = $this->reportQuery(); + + $invoicesQuery = \Modules\Invoices\Models\Invoice::query() + ->whereBetween('invoiced_at', $this->dateRange()) + ->when($this->clientId, fn ($q) => $q->where('customer_id', $this->clientId)); return trans('ip.report_summary_invoices_per_client', [ - 'clients' => $rows->count(), - 'invoices' => (int) $rows->sum('invoices_count'), + 'clients' => $query->count(), + 'invoices' => $invoicesQuery->count(), ]); }🤖 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/Company/Pages/Reports/InvoicesPerClientReport.php` around lines 33 - 41, Update summaryLine() to avoid reportQuery()->get() and calculate both the client count and total invoices_count in the database using aggregate queries. Preserve the existing translation key and summary values, ensuring clients reflects the row count and invoices remains an integer total.
🤖 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.
Nitpick comments:
In `@Modules/Core/Filament/Company/Pages/Reports/BaseTabularReportPage.php`:
- Around line 111-118: Update getClientOptions to use the query builder’s pluck
operation instead of fetching Relation models with get and transforming them via
map. Preserve the company_name ordering and return the same id-to-name option
structure expected by callers.
In `@Modules/Core/Filament/Company/Pages/Reports/InvoicedByClientReport.php`:
- Around line 33-41: Update summaryLine() to compute the client count and
invoiced_total sum through database-level aggregate queries instead of calling
get() and materializing all rows. Preserve the existing translation keys and
money formatting, using the reportQuery() builder while ensuring the count and
sum operate on the full matching dataset.
In `@Modules/Core/Filament/Company/Pages/Reports/InvoicesPerClientReport.php`:
- Around line 33-41: Update summaryLine() to avoid reportQuery()->get() and
calculate both the client count and total invoices_count in the database using
aggregate queries. Preserve the existing translation key and summary values,
ensuring clients reflects the row count and invoices remains an integer total.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eaf4be9-cd6a-429e-bf3b-2814ea042776
📒 Files selected for processing (10)
Modules/Core/Filament/Company/Pages/Reports/BaseTabularReportPage.phpModules/Core/Filament/Company/Pages/Reports/InvoicedByClientReport.phpModules/Core/Filament/Company/Pages/Reports/InvoicesPerClientReport.phpModules/Core/Filament/Company/Pages/Reports/InvoicingHistoryReport.phpModules/Core/Filament/Company/Pages/Reports/PaymentHistoryReport.phpModules/Core/Filament/Company/Pages/Reports/SalesByDateReport.phpModules/Core/Providers/CompanyPanelProvider.phpModules/Core/Tests/Feature/CompanyTabularReportsTest.phpModules/Core/resources/views/filament/company/pages/reports/tabular-report.blade.phpresources/lang/en/ip.php
4 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.
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
…ages (Phase 5) Track B of epic InvoicePlane#506, per naui95's spec: Payment History, Invoicing History, Invoiced Amount by Client, Sales by Date (paid invoices), and Invoices per Client. Each is a read-only Filament page under a Reports navigation group with a date-range filter (defaults to the current month), an optional client filter, a totals summary line, and a CSV export streaming the filtered rows. Tenant scoping comes from the BelongsToCompany global scope and is covered by a cross-company test. Includes the AbstractCompanyPanelTestCase::testLivewire fix (wrong Livewire import + withSession chained on the wrong object) — the same fix ships in the report-builder branch; whichever merges second resolves cleanly to identical content. Test scenarios follow the issue body: in-range vs out-of-range payments, per-client invoiced sum, company scoping, plus client filtering, paid-only sales, and the CSV download. Refs InvoicePlane#145 InvoicePlane#506 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
nielsdrost7
force-pushed
the
feature/145-tabular-reports
branch
from
July 24, 2026 11:09
a1b9fc4 to
4eac422
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.
Implements Track B of epic #506 — the tabular reports module per naui95's spec in #145. Independent of the Report Builder (no shared code path).
What's in here
Five read-only Filament pages in the company panel under a Reports navigation group, each with a date-range filter (defaults to the current month), an optional client filter, a totals summary line under the table, and a CSV export of the filtered rows:
withCount/withSumAll queries are tenant-scoped by the
BelongsToCompanyglobal scope. Shared behavior lives inBaseTabularReportPage(filters, CSV streaming, access control: client_admin + elevated roles).Also ships the
AbstractCompanyPanelTestCase::testLivewire()fix (broken Livewire import +withSessionchained on the wrong object) — identical fix is on the report-builder branch (underdogg-forks#2); whichever merges second resolves cleanly.Testing
Full suite green: 314 tests, 1,033 assertions, 0 failures (develop baseline: 307). The three scenarios drafted in the issue body (in-range vs out-of-range payments, per-client sum shows 1500, cross-company invisibility) plus client filtering, paid-only sales-by-date, page smoke for all five, and the CSV download assertion.
Follow-up
The issue's "later enhancement" (print/PDF export of these reports through builder templates) stays out of scope, as specified in #145.
Summary by CodeRabbit
New Features
Bug Fixes