Issue: Routes showing wrong active

[lenchantin]

Sorry for posting this here, but I can't post an issue on this repo without it wanting me to open a pull request.

I'm having an issue with the plugin reporting the incorrect active element.

Say I have two top-level menu options (no child objects):
Admin (url: /admin)
User Management (url: /admin/users)

$nav = Navigation::make() ->add('Dashboard', route('admin.dash'), fn($s) => $s->attributes(['icon' => 'fa-duotone fa-house-tree'])) ->add('User Management', route('admin.show.users'), fn($s) => $s->attributes(['icon' => 'fa-duotone fa-users-gear'])) ->tree(); return view('livewire.component.admin.dash-nav', ['navbar' => $nav]);

If I'm on the user management page, it always shows admin as active=true, but User Management is active=false.

If I add the User Management menu item first, then the Admin item, it works as expected, though I need to output the navigation item to my controller with array_reverse() -- which isn't ideal.

$nav = Navigation::make() ->add('User Management', route('admin.show.users'), fn($s) => $s->attributes(['icon' => 'fa-duotone fa-users-gear'])) ->add('Dashboard', route('admin.dash'), fn($s) => $s->attributes(['icon' => 'fa-duotone fa-house-tree'])) ->tree(); return view('livewire.component.admin.dash-nav', ['navbar' => array_reverse($nav)]);

[jeffreyvanhees]

I have the same 'problem', but this is how it was designed.

On /admin/users, /admin will be also treated as 'active' because it occurs in the URL. See https://github.com/spatie/laravel-navigation/blob/main/src/Helpers/ActiveUrlChecker.php

What I did is extending the Navigation class and inject a custom ActiveUrlChecker:

<?php

namespace App\Helpers;

use Spatie\Navigation\Helpers\ActiveUrlChecker;

class Navigation extends \Spatie\Navigation\Navigation
{
    /**
     * Override the constructor to inject a custom ActiveUrlChecker.
     */
    public function __construct()
    {
        $activeUrlChecker = new ActiveUrlChecker(request()->url(), '/admin');

        return parent::__construct($activeUrlChecker);
    }
}

[sneakylenny]

For resources this happens too 😕

Setup

For example:

// web.php

Route::resource('products', 'ProductController');

Which results in:

tim@DESKTOP-TIM:~/GitHub/myproject$ sail artisan route:list --name products

  GET|HEAD        products.................................................. products.index › ProductController@index
  POST            products.................................................. products.store › ProductController@store
  GET|HEAD        products/create ........................................ products.create › ProductController@create
  GET|HEAD        products/{product} ......................................... products.show › ProductController@show
  PUT|PATCH       products/{product} ..................................... products.update › ProductController@update
  DELETE          products/{product} ................................... products.destroy › ProductController@destroy
  GET|HEAD        products/{product}/edit .................................... products.edit › ProductController@edit

                                                                                                   Showing [7] routes

I render the navigation like this:

Navigation::make()
    ->add('Home', route('home'))
    ->add('Product', '', function (Section $section) {
    // or
    ->add('Product', route('products.index'), function (Section $section) { // Same result
        $section->add('Index', route('products.index'));
        $section->add('Create', route('products.create'));
    });

The problem

But no matter if I navigate to products.index or products.create it will always result in this output:

array:2 [▼ // app/Http/Middleware/HandleInertiaRequests.php:82
  0 => array:6 [▼
    "url" => "http://localhost/home"
    "title" => "Home"
    "active" => false
    "attributes" => []
    "children" => []
    "depth" => 0
  ]
  1 => array:6 [▼
    "url" => ""
    "title" => "Products"
    "active" => true
    "attributes" => []
    "children" => array:2 [▼
      0 => array:6 [▼
        "url" => "http://localhost/products"
        "title" => "Index"
        "active" => true <----------------------------------------- Always true in this nav item
        "attributes" => []
        "children" => []
        "depth" => 1
      ]
      1 => array:6 [▼
        "url" => "http://localhost/products/create"
        "title" => "Create"
        "active" => false <----------------------------------------- Even on http://localhost/products/create
        "attributes" => []
        "children" => []
        "depth" => 1
      ]
    ]
    "depth" => 0
  ]
]

Unless I do it like this:

$navigation = new Navigation(new ActiveUrlChecker($request->url(), '/products'));

But that would make no sense, because "/products" is only the root for this resource. That would mean I need to make a script to dynamically resolve the route resource prefix and apply that to the ActiveUrlChecker....

After some digging around I found out that this is caused by this part:

laravel-navigation/src/Helpers/ActiveUrlChecker.php

Line 51 in 31e2e32

if ($matchPath === $itemPath || Str::startsWith($matchPath, $itemPath)) {

When removing Str::startsWith($matchPath, $itemPath) it seems to work for resources. So this begs the question: Why is it there?

Workaround

The workaround for this appears to be to copy the ActiveUrlChecker class to your project and remove this condition and initialize it in the above example:

// app\Support\ActiveUrlChecker.php

// Copy of Spatie\Navigation\Helpers\ActiveUrlChecker with changes:

-if ($matchPath === $itemPath || Str::startsWith($matchPath, $itemPath)) {
+if ($matchPath === $itemPath) {

Full example: Click to expand

// app\Support\ActiveUrlChecker.php

namespace App\Support;

use Spatie\Navigation\Helpers\ActiveUrlChecker as BaseActiveUrlChecker;
use Spatie\Url\Url;
use Spatie\Navigation\Helpers\Str;

class ActiveUrlChecker extends BaseActiveUrlChecker
{
  private string $requestUrl;

  private string $rootPath;

  public function __construct(string $requestUrl, string $rootPath = '/')
  {
      $this->requestUrl = $requestUrl;
      $this->rootPath = $rootPath;
  }

  public function check(string $url): bool
  {
      $url = Url::fromString($url);
      $requestUrl = Url::fromString($this->requestUrl);

      // If the hosts don't match, this url isn't active.
      if ($url->getHost() !== $requestUrl->getHost()) {
          return false;
      }

      $rootPath = Str::ensureLeft('/', $this->rootPath);

      // All paths used in this method should be terminated by a /
      // otherwise startsWith at the end will be too greedy and
      // also matches items which are on the same level
      $rootPath = Str::ensureRight('/', $rootPath);

      $itemPath = Str::ensureRight('/', $url->getPath());

      // If this url doesn't start with the rootPath, it's inactive.
      if (! Str::startsWith($itemPath, $rootPath)) {
          return false;
      }

      $matchPath = Str::ensureRight('/', $requestUrl->getPath());

      // For the next comparisons we just need the paths, and we'll remove
      // the rootPath first.
      $itemPath = Str::removeFromStart($rootPath, $itemPath);
      $matchPath = Str::removeFromStart($rootPath, $matchPath);

      dump([$matchPath, $itemPath], ($matchPath === $itemPath));

      // If this url starts with the url we're matching with, it's active.
      if ($matchPath === $itemPath) {
          return true;
      }

      return false;
  }
}

Making use of this class:

use App\Support\ActiveUrlChecker;

$navigation = new Navigation(new ActiveUrlChecker($request->url(), $optionalPrefix = '/'));

or use @jeffreyvanhees's answer with the local class.

Conclusion

If this isn't intended behavior or if this behavior should be preferred to be optional (by adding something like an exact option) I'm willing to open a pull request to introduce those changes.
Else, I suggest using the provided workaround.

[beholdr]

Came across the same issue. Thanks for your detailed explanation.

I think exact option would be great, because patching of the ActiveUrlChecker is so inconvenient. Or at least move comparison to separate method for easy overriding.

[beholdr]

I've created PR #29 with extraction of comparison logic to a separate method. It will simplify extension of base class without copying it.