Fix menu highlighting to show correct active page by DawoudIO · Pull Request #7724 · ChurchCRM/CRM

DawoudIO · 2025-12-03T03:16:10Z

What Changed

Improve isActive() to properly match URLs with query parameters
Menu items with query params only match when params are present
Menu items without query params only match when URL has no params
Make openMenu() recursive for nested submenu support
Change submenu container from div to li for proper AdminLTE styling
Add active class to nav-link for proper visual highlighting

Type

Screenshots

Security Check

Introduces new input validation
Modifies authentication/authorization
Affects data privacy/GDPR

Code Quality

Database: Propel ORM only, no raw SQL
No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor)
Bootstrap CSS classes used
All CSS bundled via webpack

Pre-Merge

Tested locally
No new warnings
Build passes
Backward compatible (or migration documented)

- Improve isActive() to properly match URLs with query parameters - Menu items with query params only match when params are present - Menu items without query params only match when URL has no params - Make openMenu() recursive for nested submenu support - Change submenu container from div to li for proper AdminLTE styling - Add active class to nav-link for proper visual highlighting

Copilot

Pull request overview

This PR improves the menu highlighting system to properly handle URLs with query parameters and fixes the AdminLTE styling for nested submenus. The changes ensure that menu items like "View Inactive Families" (/v2/family?mode=inactive) correctly highlight when the user navigates to that specific page, while "View Active Families" (/v2/family) only highlights when no query parameters are present.

Key improvements:

Query parameter-aware URL matching that distinguishes between /v2/family and /v2/family?mode=inactive
Recursive nested submenu support for proper highlighting of deeply nested menu structures
Correct HTML structure using <li> instead of <div> for AdminLTE compatibility

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/ChurchCRM/view/MenuRenderer.php	Changed submenu container from `<div>` to `<li>` for proper AdminLTE styling, added `active` class to nav-link elements for visual highlighting
src/ChurchCRM/Config/Menu/MenuItem.php	Rewrote `isActive()` to parse and match query parameters, made `openMenu()` recursive to support nested submenu activation

Copilot · 2025-12-03T03:20:40Z

src/ChurchCRM/Config/Menu/MenuItem.php

+        $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);
+
+        // 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);


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:

Navigates to a page with query parameters (e.g., /v2/family?mode=inactive)

Verifies the correct menu item has the active class applied

Navigates to the same page without query params (e.g., /v2/family)

Verifies a different menu item is now active

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 · 2025-12-03T03:20:40Z

src/ChurchCRM/Config/Menu/MenuItem.php

+        $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);


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 · 2025-12-03T03:20:41Z

src/ChurchCRM/Config/Menu/MenuItem.php

            return false;
        }


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 · 2025-12-03T03:20:41Z

src/ChurchCRM/view/MenuRenderer.php

        ?>
        <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" : ""?>">


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" : ""?>">

DawoudIO added this to the 6.3.0 milestone Dec 3, 2025

DawoudIO requested a review from a team as a code owner December 3, 2025 03:16

Copilot AI review requested due to automatic review settings December 3, 2025 03:16

DawoudIO added UI bug labels Dec 3, 2025

DawoudIO requested review from DAcodedBEAT, MrClever, bigtigerku, grayeul and respencer and removed request for a team December 3, 2025 03:16

Copilot started reviewing on behalf of DawoudIO December 3, 2025 03:16 View session

Copilot finished reviewing on behalf of DawoudIO December 3, 2025 03:19

Copilot AI reviewed Dec 3, 2025

View reviewed changes

DawoudIO merged commit a1dd3dc into master Dec 3, 2025
13 checks passed

DawoudIO deleted the fix/menu-active-item-highlighting branch December 3, 2025 03:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix menu highlighting to show correct active page#7724

Fix menu highlighting to show correct active page#7724
DawoudIO merged 1 commit intomasterfrom
fix/menu-active-item-highlighting

DawoudIO commented Dec 3, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	<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" : ""?>">

Uh oh!

Conversation

DawoudIO commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Changed

Type

Screenshots

Security Check

Code Quality

Pre-Merge

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DawoudIO commented Dec 3, 2025 •

edited

Loading