Panel Navigation #12
Panel Navigation #12
Conversation
WalkthroughThe changes involve modifications to the configuration file Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TypeResource
participant Config
User->>Config: Request panel_navigation configuration
Config-->>User: Returns panel_navigation value (boolean or array)
User->>TypeResource: Calls shouldRegisterNavigation()
TypeResource->>Config: Checks panel_navigation value
alt If array
TypeResource->>TypeResource: Check current panel ID against array keys
TypeResource-->>User: Returns corresponding boolean value
else If boolean
TypeResource-->>User: Returns boolean value
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
🧹 Outside diff range and nitpick comments (4)
config/filament-types.php (1)
19-24: Documentation looks good, but could be more explicitThe documentation clearly explains the new configuration format. However, consider adding an example that shows both boolean and multiple panel configurations for completeness.
* Panel Navigation * Accepts: boolean OR array of panel ID with boolean * If array is empty, assumes to not display navigation item. * * Panel Example: - * 'panel_navigation' => ['admin' => TRUE]; + * Single panel: 'panel_navigation' => true + * Multiple panels: 'panel_navigation' => [ + * 'admin' => true, + * 'customer' => false, + * ];src/Filament/Resources/TypeResource.php (3)
46-53: Enhance documentation clarity with PHPDoc standards and examplesWhile the documentation provides basic information, it could be more helpful with proper PHPDoc formatting and examples.
Consider updating the documentation like this:
/** - * Config Item: `panel_navigation` - * Returns: bool - * - * Accepts: array OR bool - * - * Compares against current panel ID based on what is in the array (if provided). + * Determines if navigation should be registered based on panel configuration. + * + * @return bool Whether navigation should be registered + * + * Configuration can be either: + * - boolean: Directly controls navigation visibility + * - array: Maps panel IDs to visibility boolean values + * + * Example configuration: + * ```php + * // Simple boolean + * 'panel_navigation' => true + * + * // Panel-specific visibility + * 'panel_navigation' => [ + * 'admin' => true, + * 'customer' => false, + * ] + * ``` */
65-65: Simplify boolean handlingThe empty check is unnecessary for boolean values as FALSE is already a valid boolean.
- return (empty($configItem)) ? FALSE : $configItem; + return (bool) $configItem;
56-56: Use consistent boolean notationThe codebase mixes TRUE and true notations. Stick to lowercase for consistency with PHP conventions.
- $configItem = config('filament-types.panel_navigation', TRUE); + $configItem = config('filament-types.panel_navigation', true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
config/filament-types.php(1 hunks)src/Filament/Resources/TypeResource.php(1 hunks)
🔇 Additional comments (2)
config/filament-types.php (1)
26-26: Verify the configuration usage across the codebase
The default value of true maintains backward compatibility. Let's verify how this configuration is being used in the codebase.
✅ Verification successful
Let me gather more context about how this configuration is being used in the TypeResource file.
Configuration implementation is properly handled and flexible
The implementation in TypeResource.php shows that the panel_navigation configuration is well-designed and properly handled:
- It supports both boolean and array values for fine-grained control
- It maintains backward compatibility with a default value of
true - It includes proper panel ID comparison when array configuration is provided
- The configuration is used to determine navigation visibility through
shouldRegisterNavigation()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the new panel_navigation config is being used
# Look for references to the old and new config keys
echo "Checking for old config key usage..."
rg -l "show_navigation"
echo "Checking for new config key usage..."
rg -l "panel_navigation"
# Look for Filament panel registrations to ensure all panels are covered
echo "Checking panel registrations..."
ast-grep --pattern 'Filament::registerPanel($_)'
Length of output: 404
Script:
#!/bin/bash
# Check the usage context of panel_navigation in TypeResource
rg -A 5 -B 5 "panel_navigation" src/Filament/Resources/TypeResource.php
# Look for any navigation-related methods or configurations
ast-grep --pattern 'function configureNavigation() { $$$ }'
# Check if there are any other navigation-related configurations
rg -l "navigation" --type php
Length of output: 970
src/Filament/Resources/TypeResource.php (1)
54-67: Verify configuration updates across the application
The change from show_navigation to panel_navigation might affect existing installations.
Let's check for any remaining references to the old configuration key:
| if (is_array($configItem) && !empty($configItem)) { | ||
| foreach (config('filament-types.panel_navigation', true) as $key => $val) { | ||
| if (Filament::getCurrentPanel()->getId() === $key) { | ||
| return $val; | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Fix array handling logic and optimize config access
The current implementation has several issues:
- Redundant config fetch in the loop
- Missing default return value for array case
- Potential null panel ID handling
Consider this improved implementation:
- if (is_array($configItem) && !empty($configItem)) {
- foreach (config('filament-types.panel_navigation', true) as $key => $val) {
- if (Filament::getCurrentPanel()->getId() === $key) {
- return $val;
- }
- }
+ if (is_array($configItem)) {
+ $currentPanelId = Filament::getCurrentPanel()?->getId();
+ return $configItem[$currentPanelId] ?? false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (is_array($configItem) && !empty($configItem)) { | |
| foreach (config('filament-types.panel_navigation', true) as $key => $val) { | |
| if (Filament::getCurrentPanel()->getId() === $key) { | |
| return $val; | |
| } | |
| } | |
| } else { | |
| if (is_array($configItem)) { | |
| $currentPanelId = Filament::getCurrentPanel()?->getId(); | |
| return $configItem[$currentPanelId] ?? false; | |
| } else { |
| public static function shouldRegisterNavigation(): bool | ||
| { | ||
| $configItem = config('filament-types.panel_navigation', TRUE); | ||
|
|
||
| if (is_array($configItem) && !empty($configItem)) { | ||
| foreach (config('filament-types.panel_navigation', true) as $key => $val) { | ||
| if (Filament::getCurrentPanel()->getId() === $key) { | ||
| return $val; | ||
| } | ||
| } | ||
| } else { | ||
| return (empty($configItem)) ? FALSE : $configItem; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add missing Filament class import
The Filament class is used but not imported.
Add this import at the top of the file:
namespace TomatoPHP\FilamentTypes\Filament\Resources;
+use Filament\Facades\Filament;
use Filament\Forms\Form;Committable suggestion skipped: line range outside the PR's diff.
|
Fixed: Use Filament\Facades\Filament;As well as a small bug where it would not default to FALSE if the current panel ID was not in the provided array. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/Filament/Resources/TypeResource.php (1)
47-54: Improve method documentationThe current documentation could be enhanced with better PHPDoc formatting and more detailed information.
Here's an improved version:
/** - * Config Item: `panel_navigation` - * Returns: bool - * - * Accepts: array OR bool - * - * Compares against current panel ID based on what is in the array (if provided). + * Determines if navigation should be registered for the current panel. + * + * This method checks the 'panel_navigation' configuration to determine if + * navigation should be shown. The configuration can be either: + * - boolean: Applies to all panels + * - array: Panel-specific settings where keys are panel IDs and values are booleans + * + * @example + * // config/filament-types.php + * 'panel_navigation' => [ + * 'admin' => true, + * 'customer' => false, + * ] + * + * @return bool True if navigation should be registered for the current panel */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Filament/Resources/TypeResource.php(2 hunks)
🔇 Additional comments (2)
src/Filament/Resources/TypeResource.php (2)
12-12: LGTM!
The Filament facade import has been added as suggested in the past review.
55-70: Verify configuration usage across panels
Let's ensure the configuration is properly structured in all panel configurations.
✅ Verification successful
Implementation correctly handles panel-specific navigation configuration
The implementation is correctly structured to handle both boolean and panel-specific array configurations as defined in the config file. The code matches the documented configuration format:
- The default configuration is set to
true(show navigation for all panels) - The code properly handles array configurations for panel-specific visibility (e.g.,
['admin' => true]) - The empty array case is handled correctly to hide navigation
- The implementation correctly uses
Filament::getCurrentPanel()->getId()to match against panel IDs
The configuration file's documentation and the implementation are in sync, providing a flexible way to control navigation visibility per panel.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check panel configuration structure across the codebase
# Search for panel configuration files
echo "Searching for panel configuration files..."
rg -l "panel_navigation|show_navigation" config/
# Check for panel registration to understand available panel IDs
echo "Checking panel registration..."
rg -A 5 "Panel::make|::configurePanel"
Length of output: 283
Script:
#!/bin/bash
# Let's examine the configuration file and panel setup more thoroughly
# Check the content of the configuration file
echo "Content of config/filament-types.php:"
cat config/filament-types.php
# Look for panel-related code in the codebase
echo -e "\nSearching for panel-related code:"
rg -A 3 "getCurrentPanel|Panel::make|PanelProvider"
# Look for any test cases related to panel navigation
echo -e "\nSearching for related test cases:"
fd -e php -e test.php --exec grep -l "panel_navigation\|shouldRegisterNavigation\|TypeResource" {}
Length of output: 5987
| public static function shouldRegisterNavigation(): bool | ||
| { | ||
| $configItem = config('filament-types.panel_navigation', TRUE); | ||
|
|
||
| if (is_array($configItem) && !empty($configItem)) { | ||
| foreach (config('filament-types.panel_navigation', true) as $key => $val) { | ||
| if (Filament::getCurrentPanel()->getId() === $key) { | ||
| return $val; | ||
| } else { | ||
| return FALSE; | ||
| } | ||
| } | ||
| } else { | ||
| return (empty($configItem)) ? FALSE : $configItem; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Fix logical issues in navigation registration method
The current implementation has several issues that need to be addressed:
- The foreach loop's else clause causes early return, making it only process the first item
- Redundant config fetch inside the loop
- Missing null safety check for getCurrentPanel()
- Inconsistent boolean casing
- Unnecessary combination of is_array and empty checks
Here's the corrected implementation:
public static function shouldRegisterNavigation(): bool
{
- $configItem = config('filament-types.panel_navigation', TRUE);
+ $configItem = config('filament-types.panel_navigation', true);
- if (is_array($configItem) && !empty($configItem)) {
- foreach (config('filament-types.panel_navigation', true) as $key => $val) {
- if (Filament::getCurrentPanel()->getId() === $key) {
- return $val;
- } else {
- return FALSE;
- }
- }
- } else {
- return (empty($configItem)) ? FALSE : $configItem;
- }
+ if (is_array($configItem)) {
+ $currentPanel = Filament::getCurrentPanel();
+ if (!$currentPanel) {
+ return false;
+ }
+ return $configItem[$currentPanel->getId()] ?? false;
+ }
+
+ return (bool) $configItem;
}This implementation:
- Removes redundant config fetch
- Adds null safety check for getCurrentPanel()
- Uses array key lookup instead of loop
- Consistently uses lowercase booleans
- Properly handles array and boolean cases
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function shouldRegisterNavigation(): bool | |
| { | |
| $configItem = config('filament-types.panel_navigation', TRUE); | |
| if (is_array($configItem) && !empty($configItem)) { | |
| foreach (config('filament-types.panel_navigation', true) as $key => $val) { | |
| if (Filament::getCurrentPanel()->getId() === $key) { | |
| return $val; | |
| } else { | |
| return FALSE; | |
| } | |
| } | |
| } else { | |
| return (empty($configItem)) ? FALSE : $configItem; | |
| } | |
| } | |
| public static function shouldRegisterNavigation(): bool | |
| { | |
| $configItem = config('filament-types.panel_navigation', true); | |
| if (is_array($configItem)) { | |
| $currentPanel = Filament::getCurrentPanel(); | |
| if (!$currentPanel) { | |
| return false; | |
| } | |
| return $configItem[$currentPanel->getId()] ?? false; | |
| } | |
| return (bool) $configItem; | |
| } |
The current config item of "show_navigation" does not currently function (at least not that I could tell) - further, it does not take into consideration multiple panels.
This PR will add the functionaility to allow to control whether or not the navigation item is visible for multiple panels based on the panel ID.
Summary by CodeRabbit
New Features
panel_navigation, allowing for more flexible navigation control.Documentation
panel_navigationkey with examples.