Feature: Ability to have any number of datatable per page #2
Conversation
WalkthroughThe changes introduce support for multiple datatable instances on a page by namespacing datatable state and interactions using a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Datatable (React)
participant Server (PHP InertiaDatatable)
User->>Datatable (React): Interacts (sort/filter/paginate)
Datatable (React)->>Server (PHP InertiaDatatable): POST { [name]: queryParams }
Server (PHP InertiaDatatable)->>Server (PHP InertiaDatatable): Handle request using $name
Server (PHP InertiaDatatable)-->>Datatable (React): Respond with { [name]: props }
Datatable (React)-->>User: Render updated datatable
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/Unit/RelationshipColumnTest.php (1)
94-170: 🛠️ Refactor suggestionHelper lacks ability to specify datatable “name”
setDatatableRequest(['search' => 'John']);wraps the payload under'dt'.
Now that multiple tables can coexist, tests exercising a non-default table name cannot be written without duplicating logic. Extend the helper:- protected function setDatatableRequest(array $data): void + protected function setDatatableRequest(array $data, string $key = 'dt'): voidand update calls:
$this->setDatatableRequest(['search' => 'John'], 'users_table');
🧹 Nitpick comments (8)
resources/js/components/datatable/types.ts (1)
123-123: Add minimal JSDoc for the new propA one-liner description (e.g. “Unique identifier used to namespace datatable state when multiple tables exist on the same page.”) will improve DX and avoid misuse (e.g. two tables accidentally sharing the same key).
- name: string; + /** + * Unique identifier to namespace this datatable’s state + * when several instances live on the same page. + */ + name: string;tests/Traits/WithDatatableRequest.php (2)
10-13: Replacing the whole request may wipe unrelated parameters
request()->replace(['dt' => $data]);completely overrides the current request payload, potentially erasing headers or additional query/body params that individual tests might rely on in the future.Consider a safer merge:
- request()->replace(['dt' => $data]); + $current = request()->all(); + request()->replace(array_merge($current, ['dt' => $data]));…or restrict this helper to
TestRequest::create()to avoid mutating the global request altogether.
5-13: Missing PHPDoc & visibility for the traitAdding a file-level docblock and a PHPDoc for
setDatatableRequest()clarifies intent and static-analysis expectations:+/** + * Helper trait for unit tests to scaffold datatable-specific request payloads. + */ trait WithDatatableRequest { /** - * Helper method to set the request data correctly for the datatable + * Replace the current HTTP request payload with the provided datatable data. + * + * @param array<string,mixed> $data */ protected function setDatatableRequest(array $data): voidtests/Unit/InertiaDatatableExportTest.php (1)
55-57: Duplicate request-building logic – leverage the new trait for consistencyYou’ve introduced
WithDatatableRequest, but this test still manually instantiates aRequest. Using the trait reduces boilerplate and keeps all tests aligned with future request-format tweaks.- // Make a request with the export parameter - $request = new Request(['dt' => ['export' => true]]); - $this->app->instance(Request::class, $request); + // Make a request with the export parameter + $this->setDatatableRequest(['export' => true]);(remember to
use Tests\Traits\WithDatatableRequest;and include the trait.)tests/Unit/InertiaDatatableHandleExportTest.php (1)
70-74: Same request-building duplication as aboveConsider switching to
setDatatableRequest()here (and in the other two request creations) to avoid three slightly different manual constructions.tests/Unit/RelationshipColumnTest.php (2)
11-14: Unused import
Illuminate\Http\Requestis no longer used after migrating to the helper.-use Illuminate\Http\Request;
18-19: Trait order – cosmeticsLaravel sniffs recommend placing framework traits (
WithTestModels) after custom traits for readability, but this is optional.src/InertiaDatatable.php (1)
188-207: Consider using consistent default name.The static
make()method uses 'datatable' as the default name (line 196), while the class property defaults to 'dt' (line 31). Consider using the same default for consistency.- $dataTable->name = $name ?? 'datatable'; + $dataTable->name = $name ?? 'dt';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
resources/js/components/datatable/index.tsx(2 hunks)resources/js/components/datatable/types.ts(1 hunks)src/InertiaDatatable.php(19 hunks)tests/Traits/WithDatatableRequest.php(1 hunks)tests/Unit/InertiaDatatableExportTest.php(2 hunks)tests/Unit/InertiaDatatableHandleExportTest.php(4 hunks)tests/Unit/InertiaDatatableTest.php(40 hunks)tests/Unit/RelationshipColumnTest.php(5 hunks)tests/Unit/SessionMethodsTest.php(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
resources/js/components/datatable/index.tsx (1)
resources/js/components/datatable/types.ts (2)
DatatableProps(121-125)PageProps(85-119)
🪛 Biome (1.9.4)
resources/js/components/datatable/index.tsx
[error] 19-19: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
🔇 Additional comments (9)
resources/js/components/datatable/types.ts (1)
121-125:Details
❌ Incorrect review comment
Required
nameprop may introduce silent breakages; audit call-sitesMaking
namemandatory is a breaking API change. Any existing<Datatable>usages that were not updated will now fail the type–checker (or worse, at runtime if compiled JS is consumed elsewhere).
Please run a quick repo-wide search to ensure every instantiation passes aname, and consider markingnameas optional for one minor release with a deprecation warning to ease migration.
🏁 Script executed:
#!/bin/bash # Search for all JSX/TSX instantiations of the Datatable component rg -n "<Datatable" --glob "*.tsx" --glob "*.jsx"Length of output: 1302
🏁 Script executed:
#!/bin/bash # Display the top of the Datatable component file to verify its export sed -n '1,50p' resources/js/components/datatable/index.tsx # Search for any imports of the Datatable component elsewhere in the repo rg -n "import .*Datatable" --glob "*.ts*" --glob "*.js*"Length of output: 2677
No
<Datatable>usages found—makingnamerequired is safe
A repo-wide search for<Datatable>tags in all.tsx/.jsxfiles returned no instantiations outside the component’s own file, so there are no call-sites to update and no breaking change.Likely an incorrect or invalid review comment.
tests/Unit/SessionMethodsTest.php (1)
9-9: LGTM! Consistent refactoring to support namespaced datatable requests.The replacement of
request()->replace()with$this->setDatatableRequest()throughout the test class properly aligns with the new architecture where datatable requests are scoped under a namespace key.Also applies to: 16-16, 34-34, 41-41, 58-61, 70-70, 88-90, 97-97, 115-117, 124-124, 141-143, 149-151, 205-207, 217-219, 246-248, 254-254
resources/js/components/datatable/index.tsx (2)
19-19: Skip static analysis hint - false positive.The
name: namesyntax in the component props is intentional for clarity and consistency.🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
279-288: Well-implemented namespacing for multiple datatable support.The request payload structure and the
onlyarray correctly namespace all datatable-related data under thenamekey, enabling multiple datatables to coexist on the same page without conflicts.src/InertiaDatatable.php (2)
31-31: Good implementation of namespaced request handling.Using a Collection scoped to the datatable name is a clean approach that provides better type safety and enables multiple datatables per page. The default name 'dt' maintains backward compatibility.
Also applies to: 62-65
265-265: Correctly includes datatable name in props.Including the
namein the props array enables the frontend component to access the correct namespace for this datatable instance.tests/Unit/InertiaDatatableTest.php (3)
13-13: Consistent test refactoring to support namespaced requests.The test file has been thoroughly updated to use the
WithDatatableRequesttrait and helper method, ensuring all tests work correctly with the new namespaced request structure.Also applies to: 25-25, 83-83, 138-138, 151-151, 228-228, 240-240, 272-272, 291-291, 304-304, 396-396, 408-408, 420-420, 439-439, 472-472, 489-489, 502-502, 1070-1070, 1168-1168, 1229-1229, 1261-1261, 1265-1265
536-540: Action handling tests correctly updated for namespaced requests.All action-related request payloads are properly nested under the 'dt' key, ensuring the tests accurately reflect the new request structure.
Also applies to: 562-566, 586-590, 610-613, 639-643, 678-682, 724-728, 751-754, 781-785, 816-820, 1301-1305, 1340-1344, 1370-1374, 1406-1410, 1444-1448, 1474-1478, 1515-1519, 1553-1557
1530-1566: Excellent test coverage for new functionality.The new tests thoroughly cover:
- Column actions requiring a single ID (returns null for multiple IDs)
- Static
make()method error handling when table not set- Normal operation of static
make()method- Export handling through static
make()methodThis ensures the new features are well-tested and maintainable.
Also applies to: 1568-1655
1 participant
Summary by CodeRabbit