Skip to content

Fix menu highlighting to show correct active page #7724

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.

Sign up for GitHub

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

Merged
DawoudIO merged 1 commit into from
Dec 3, 2025
Merged

Fix menu highlighting to show correct active page #7724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion src/ChurchCRM/Config/Menu/MenuItem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,14 @@ public function isVisible(): bool
public function openMenu(): bool
{
foreach ($this->subItems as $item) {
// Check if this child is active
if ($item->isActive()) {
return true;
}
// Recursively check if any nested submenu has an active item
if ($item->isMenu() && $item->openMenu()) {
return true;
}
}

return false;
Expand All @@ -132,6 +137,41 @@ public function isActive(): bool
return false;
}
Comment on lines 137 to 138
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a docblock comment above the isActive() method to document the new query parameter matching behavior:

/**
 * Check if this menu item should be highlighted as active.
 * 
 * Matching logic:
 * - Paths must match exactly
 * - If menu item has query params, all must be present in current URL (subset matching)
 * - If menu item has no query params, current URL must also have no query params
 * 
 * @return bool True if this menu item matches the current page
 */

This would help future maintainers understand the matching behavior without reading through the implementation.

Copilot uses AI. Check for mistakes.

return $_SERVER['REQUEST_URI'] == $this->getURI();
$menuUri = $this->getURI();
$currentUri = $_SERVER['REQUEST_URI'];

// Parse both URIs
$currentPath = parse_url($currentUri, PHP_URL_PATH);
$menuPath = parse_url($menuUri, PHP_URL_PATH);
$menuQuery = parse_url($menuUri, PHP_URL_QUERY);
$currentQuery = parse_url($currentUri, PHP_URL_QUERY);
Comment on lines +144 to +147
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parse_url() function can return false on malformed URLs or null for missing components. While unlikely with internal menu URIs, consider adding null safety checks:

$currentPath = parse_url($currentUri, PHP_URL_PATH) ?? '';
$menuPath = parse_url($menuUri, PHP_URL_PATH) ?? '';

This prevents potential type errors if either URL is malformed or missing a path component.

Suggested change
$currentPath = parse_url($currentUri, PHP_URL_PATH);
$menuPath = parse_url($menuUri, PHP_URL_PATH);
$menuQuery = parse_url($menuUri, PHP_URL_QUERY);
$currentQuery = parse_url($currentUri, PHP_URL_QUERY);
$currentPath = parse_url($currentUri, PHP_URL_PATH) ?? '';
$menuPath = parse_url($menuUri, PHP_URL_PATH) ?? '';
$menuQuery = parse_url($menuUri, PHP_URL_QUERY) ?? '';
$currentQuery = parse_url($currentUri, PHP_URL_QUERY) ?? '';

Copilot uses AI. Check for mistakes.

// Paths must match first
if ($currentPath !== $menuPath) {
return false;
}

// If menu item has query params, check they all match
if (!empty($menuQuery)) {
if (empty($currentQuery)) {
return false;
}

parse_str($menuQuery, $menuParams);
parse_str($currentQuery, $currentParams);

// Check if all menu params exist in current params with same values
foreach ($menuParams as $key => $value) {
if (!isset($currentParams[$key]) || $currentParams[$key] !== $value) {
return false;
}
}

return true;
}

// Menu item has NO query params - only match if current URL also has no query params
// This prevents "/v2/family" from matching "/v2/family?mode=inactive"
return empty($currentQuery);
Comment on lines +140 to +175
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The improved URL matching logic with query parameter support is a significant behavioral change for menu highlighting, but there are no UI tests to verify this works correctly. Consider adding a Cypress test in cypress/e2e/ui/ that:

  1. Navigates to a page with query parameters (e.g., /v2/family?mode=inactive)
  2. Verifies the correct menu item has the active class applied
  3. Navigates to the same page without query params (e.g., /v2/family)
  4. Verifies a different menu item is now active
  5. Tests nested submenu activation with the new recursive openMenu() logic

Example test structure:

describe('Menu Highlighting', () => {
  beforeEach(() => {
    cy.setupAdminSession();
  });

  it('should highlight menu item with query parameters', () => {
    cy.visit('/v2/family?mode=inactive');
    cy.get('.nav-link.active').should('contain', 'View Inactive Families');
  });

  it('should highlight menu item without query parameters', () => {
    cy.visit('/v2/family');
    cy.get('.nav-link.active').should('contain', 'View Active Families');
  });
});

Copilot uses AI. Check for mistakes.
}
}
8 changes: 4 additions & 4 deletions src/ChurchCRM/view/MenuRenderer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ private static function renderMenuItem(MenuItem $menuItem): void
{
?>
<li class="nav-item<?= $menuItem->isActive() ? " active" : ""?>">
<a href="<?= htmlspecialchars($menuItem->getURI(), ENT_QUOTES, 'UTF-8') ?>" <?= $menuItem->isExternal() ? "target='_blank'" : "" ?> class="nav-link">
<a href="<?= htmlspecialchars($menuItem->getURI(), ENT_QUOTES, 'UTF-8') ?>" <?= $menuItem->isExternal() ? "target='_blank'" : "" ?> class="nav-link<?= $menuItem->isActive() ? " active" : ""?>">
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using target='_blank' on external links without rel='noopener noreferrer' enables reverse tabnabbing. An attacker controlling a getURI() target can open in a new tab and then use window.opener to navigate the original app window to a phishing page. Add rel='noopener noreferrer' when isExternal() is true, e.g., target='_blank' rel='noopener noreferrer'.

Suggested change
<a href="<?= htmlspecialchars($menuItem->getURI(), ENT_QUOTES, 'UTF-8') ?>" <?= $menuItem->isExternal() ? "target='_blank'" : "" ?> class="nav-link<?= $menuItem->isActive() ? " active" : ""?>">
<a href="<?= htmlspecialchars($menuItem->getURI(), ENT_QUOTES, 'UTF-8') ?>" <?= $menuItem->isExternal() ? "target='_blank' rel='noopener noreferrer'" : "" ?> class="nav-link<?= $menuItem->isActive() ? " active" : ""?>">

Copilot uses AI. Check for mistakes.
<i class='nav-icon fa <?= $menuItem->getIcon() ?>'></i>
<p>
<span><?= htmlspecialchars($menuItem->getName()) ?></span>
Expand All @@ -41,8 +41,8 @@ private static function renderMenuItem(MenuItem $menuItem): void
private static function renderSubMenuItem(MenuItem $menuItem): void
{
?>
<div class="nav-item<?= $menuItem->openMenu() ? " menu-open active" : "" ?>">
<a href="#" class="nav-link">
<li class="nav-item<?= $menuItem->openMenu() ? " menu-open" : "" ?>">
<a href="#" class="nav-link<?= $menuItem->openMenu() ? " active" : "" ?>">
<i class="nav-icon fa <?= $menuItem->getIcon() ?>"></i>
<p>
<span><?= htmlspecialchars($menuItem->getName()) ?></span>
Expand All @@ -63,7 +63,7 @@ private static function renderSubMenuItem(MenuItem $menuItem): void
}
} ?>
</ul>
</div>
</li>
<?php
}

Expand Down
Loading