-
-
Notifications
You must be signed in to change notification settings - Fork 254
Allow to "embed" server list #1860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThe ServerResource class adds a navigation icon property, a getNavigationBadge() method returning the current user's directly accessible server count as a string, and an embedServerList(bool $condition = true) method to toggle slug registration and navigation presence. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ServerResource
participant ServerModel as Server
Note over ServerResource: Navigation badge retrieval flow
User->>ServerResource: request navigation badge
ServerResource->>Server: query directlyAccessible()->where('owner_id', user.id)->count()
Server-->>ServerResource: count
ServerResource-->>User: return count as string
Note over ServerResource: embedServerList(bool) flow
User->>ServerResource: call embedServerList(condition)
alt condition = true
ServerResource->>ServerResource: set slug = null\nregister navigation
else condition = false
ServerResource->>ServerResource: set slug = '/' \nunregister navigation
end
Pre-merge checks✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Filament/App/Resources/Servers/ServerResource.php (1)
36-40: Add return type and consider the implications of mutating static properties.The method lacks a return type declaration and modifies static properties at runtime, which can cause unexpected behavior in long-running processes, tests, or when the resource class is instantiated multiple times.
Apply this diff to add the return type:
-public static function embedServerList(bool $condition = true) +public static function embedServerList(bool $condition = true): void { static::$slug = $condition ? null : '/'; static::$shouldRegisterNavigation = $condition; }Alternatively, return
staticto enable method chaining:-public static function embedServerList(bool $condition = true) +public static function embedServerList(bool $condition = true): static { static::$slug = $condition ? null : '/'; static::$shouldRegisterNavigation = $condition; + return new static(); }Consider documenting the method's purpose with a docblock to clarify when and how plugins should use it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/App/Resources/Servers/ServerResource.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/App/Resources/Servers/ServerResource.php (1)
app/Models/User.php (1)
directAccessibleServers(284-291)
🔇 Additional comments (1)
app/Filament/App/Resources/Servers/ServerResource.php (1)
13-13: LGTM!The navigation icon assignment is appropriate for a server resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/Filament/App/Resources/Servers/ServerResource.php (1)
19-22: Cache the user instance and clarify filtering intent.The same performance and logic concerns previously raised still apply:
user()is called twice, and the additionalwhere('owner_id', ...)filter narrowsdirectAccessibleServers()to only owned servers, excluding subuser access.
🧹 Nitpick comments (1)
app/Filament/App/Resources/Servers/ServerResource.php (1)
36-40: Add documentation for plugin initialization timing.The method correctly toggles between embedded and default modes by modifying static properties. However, consider these edge cases:
- Timing dependency: This must be called before Filament registers the resource, otherwise changes won't take effect.
- Multiple plugins: If multiple plugins call this method, the last call wins—there's no conflict detection.
Consider adding a docblock to clarify usage:
+ /** + * Toggle server list embedding in navigation. + * Must be called during plugin initialization, before resource registration. + * + * @param bool $condition True to embed (show in nav), false for default behavior + */ public static function embedServerList(bool $condition = true): void { static::$slug = $condition ? null : '/'; static::$shouldRegisterNavigation = $condition; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/App/Resources/Servers/ServerResource.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/App/Resources/Servers/ServerResource.php (1)
app/Models/User.php (1)
directAccessibleServers(284-291)
🔇 Additional comments (1)
app/Filament/App/Resources/Servers/ServerResource.php (1)
13-13: LGTM! Valid Filament navigation icon.The
navigationIconproperty is correctly defined with a valid Tabler icon reference for Filament resources.
Isn't really doing anything on it's own, but allows plugins to "embed" the server list as normal page when expanding the
apppanel.