Fix refresh action for egg index select & add refresh action to allocation ip selects #1736
Conversation
📝 WalkthroughWalkthroughSelect option sources were made lazy by switching to closures for allocations and egg imports. Refresh hint actions were added: allocation IP selects clear the node IP cache, and the egg import select runs an Artisan command to rebuild the egg index. A "refresh" translation key was added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant F as Filament Form (Allocation IP Select)
participant C as Cache
participant N as Node Model
U->>F: Click "Refresh" hint action
Note right of F #eeeeff: Hint action clears node IP cache
F->>C: forget("nodes.{node_id}.ips")
F->>N: fetch ipAddresses()
N-->>F: [ips]
F->>F: rebuild options via closure
F-->>U: Updated IP options
sequenceDiagram
autonumber
actor U as User
participant F as ImportEggAction
participant A as Artisan
participant CMD as UpdateEggIndexCommand
participant C as Cache
U->>F: Click "Refresh" hint action
Note right of F #eeffee: Action calls Artisan command to rebuild index
F->>A: call(UpdateEggIndexCommand)
A->>CMD: execute
CMD->>C: update cache('eggs.index')
U->>F: Open select / reload options
F->>C: get cache('eggs.index') via closure
C-->>F: [egg repos]
F-->>U: Updated GitHub options
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php (1)
95-103: Verify the translation key path.The tooltip references
trans('admin/admin/node.refresh')with a double "admin/" path, which appears incorrect. Based on the translation filelang/en/admin/node.php, it should likely betrans('admin/node.refresh').- ->tooltip(trans('admin/admin/node.refresh')) + ->tooltip(trans('admin/node.refresh'))app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (1)
85-93: Fix the translation key path.Same issue as in the nodes relation manager - the tooltip uses an incorrect double "admin/" path.
- ->tooltip(trans('admin/admin/node.refresh')) + ->tooltip(trans('admin/node.refresh'))app/Filament/Admin/Resources/Servers/Pages/CreateServer.php (1)
227-235: Fix the translation key path.Same translation key issue as found in the other components.
- ->tooltip(trans('admin/admin/node.refresh')) + ->tooltip(trans('admin/node.refresh'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php(1 hunks)app/Filament/Admin/Resources/Servers/Pages/CreateServer.php(1 hunks)app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php(1 hunks)app/Filament/Components/Actions/ImportEggAction.php(1 hunks)lang/en/admin/node.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php (1)
app/Models/Node.php (1)
ipAddresses(379-401)
app/Filament/Admin/Resources/Servers/Pages/CreateServer.php (1)
app/Models/Node.php (2)
Node(59-402)ipAddresses(379-401)
app/Filament/Components/Actions/ImportEggAction.php (1)
app/Console/Commands/Egg/UpdateEggIndexCommand.php (1)
UpdateEggIndexCommand(8-46)
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (1)
app/Models/Node.php (1)
ipAddresses(379-401)
🔇 Additional comments (10)
lang/en/admin/node.php (1)
40-40: LGTM!The translation entry is correctly added and aligns with the refresh functionality being introduced across the codebase. The 'refresh' translation key provides proper localization support for the refresh buttons that clear cached data using Laravel's cache()->forget() method.
app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php (2)
88-88: Good change to lazy evaluation!Converting the options from eager evaluation to a closure improves performance by deferring the expensive
ipAddresses()call until the field is actually rendered.
100-102: Effective cache invalidation implementation!The refresh action properly clears the specific cache key that corresponds to the node's IP addresses, ensuring users get fresh data when needed. This aligns with Laravel's recommended practice of using Cache::forget() to remove specific cache items.
app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php (2)
80-80: LGTM! Consistent lazy evaluation implementation.The conversion to closure-based options is consistent with the pattern implemented across other components in this PR and improves performance.
90-92: Proper cache key targeting!The cache invalidation correctly targets the specific node's IP cache using the server's node relationship, maintaining data integrity.
app/Filament/Components/Actions/ImportEggAction.php (3)
128-128: Consistent lazy evaluation pattern!The migration to closure-based options follows the same performance optimization pattern implemented in the allocation IP selects.
133-141: Verify the Artisan command execution approach.The refresh action directly calls the Artisan command, which will execute synchronously and might block the UI for users. Consider if this could cause performance issues for the user experience, especially if the command takes time to fetch and process the egg index data.
The
UpdateEggIndexCommandfetches data from a remote GitHub URL, which could potentially be slow or fail. Consider:
- Adding error handling for the refresh action
- Potentially showing a loading state or notification to the user
- Evaluating if this should run asynchronously
Based on the relevant code snippet, the command makes an HTTP request to GitHub and processes JSON data, which could take several seconds.
137-137: Translation key present — no action required. Found trans('admin/egg.import.refresh') in lang/*/admin/egg.php (e.g. lang/en/admin/egg.php:22).app/Filament/Admin/Resources/Servers/Pages/CreateServer.php (2)
221-221: Consistent implementation pattern!The lazy evaluation approach matches the implementation in the relation managers, maintaining consistency across the codebase.
232-234: Proper cache key construction!The cache invalidation correctly uses the current form state to build the cache key, ensuring the right node's IP cache is cleared.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Admin/Resources/Servers/Pages/CreateServer.php (1)
221-221: Prefer explicit Get parameter and return array for options.Avoid capturing $get; pass it explicitly. Also return an array to align with Filament expectations and your other options closures.
- ->options(fn () => collect(Node::find($get('node_id'))?->ipAddresses())->mapWithKeys(fn (string $ip) => [$ip => $ip])) + ->options(fn (Get $get) => collect(Node::find($get('node_id'))?->ipAddresses()) + ->mapWithKeys(fn (string $ip) => [$ip => $ip]) + ->toArray() + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php(1 hunks)app/Filament/Admin/Resources/Servers/Pages/CreateServer.php(1 hunks)app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/Filament/Admin/Resources/Nodes/RelationManagers/AllocationsRelationManager.php
- app/Filament/Admin/Resources/Servers/RelationManagers/AllocationsRelationManager.php
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Admin/Resources/Servers/Pages/CreateServer.php (1)
app/Models/Node.php (2)
Node(59-402)ipAddresses(379-401)
🔇 Additional comments (1)
app/Filament/Admin/Resources/Servers/Pages/CreateServer.php (1)
227-235: Accept Get $get in the action; add a success notification; translation found in en
- Accept Get $get in the action closure instead of capturing $get to avoid stale state.
- Optional: send a success Notification after cache()->forget(...) so users see the list was refreshed.
- Found translation: lang/en/admin/node.php:40 -> 'refresh' => 'Refresh' — add to other locales if needed.
- ->hintAction( - Action::make('refresh') - ->iconButton() - ->icon('tabler-refresh') - ->tooltip(trans('admin/node.refresh')) - ->action(function () use ($get) { - cache()->forget("nodes.{$get('node_id')}.ips"); - }) - ) + ->hintAction( + Action::make('refresh') + ->iconButton() + ->icon('tabler-refresh') + ->tooltip(trans('admin/node.refresh')) + ->action(function (Get $get) { + cache()->forget("nodes.{$get('node_id')}.ips"); + // Optional: notify success + // Notification::make()->title('IP list refreshed')->success()->send(); + }) + )
Closes #1716