Skip to content

Conversation

@yunarch
Copy link
Contributor

@yunarch yunarch commented Dec 2, 2025

  • Needs to be tested locally by reviewer

Description

This PR adds support for configuring sidebar menu items to match their links either exactly or partially (the current behavior).

Problem

Currently, a menu item is considered active if its link is included anywhere in the current URL. This results in overly broad matches. For example, consider the following menu item links:

menuItem1: /projects
menuItem2: /projects/id-1234
menuItem3: /projects/id-1234/some-internal-project-section
menuItem4: /projects/id-1234/some-internal-project-section/id-123

With the existing behavior and:

currentUrl = /projects/id-1234/some-internal-project-section/id-123

All menu items above would be marked as active because each item's link is a substring of the current URL. This leads to multiple items being highlighted when only the most specific one (menuItem4) should be active, at least for this case.

Why this matters

When a menu item is active, clicking it does not trigger navigation (which is correct). However, being “inside” a menu item's path does not always mean that the item itself should be marked active.

For example, imagine a sidebar with a single Projects menu item that leads to a list of all projects. A user might navigate deep into a specific project’s pages and then want to return to the main Projects page by clicking the sidebar item. With the current behavior, because all deeper project pages share the /projects prefix, the Projects item becomes active even while viewing a specific project. This makes it impossible for the user to navigate back using the sidebar, since clicking an already-active item does nothing.

To work around this today, the only option would be to change the URL structure so each section uses different prefixes, such as:

menuItem1: /projects
menuItem2: /project/id-1234
menuItem3: /some-internal-project-sections/id-1234
menuItem4: /some-internal-project-section/id-123

But this forces URLs into unnatural or inconsistent patterns solely to avoid the matching issue, and breaks the idea of meaningful, hierarchical deep linking.

What This PR Changes

This PR introduces a configuration option that allows each menu item to define how it should be matched against the current URL:

  • Exact match: the item is active only when the current URL matches its link exactly.
  • Partial match (existing behavior): the item is active when the current URL starts with its link.

This gives developers fine-grained control over how active states are determined and prevents sidebar items from being incorrectly highlighted or becoming unusable due to prefix-based collisions.

Partial match will be the default one to not break backward compatibility.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for exact URL matching in sidebar menu items to address issues where parent menu items become incorrectly active when viewing nested pages. The feature allows developers to configure whether each menu item should match URLs exactly or partially (the default behavior for backward compatibility).

Key changes:

  • Added optional exact boolean property to sidebar menu item types with documentation
  • Exported isExactUrl utility function for use in menu item components
  • Updated BasicMenuItem and CollapsableMenuItem to conditionally use exact or partial URL matching based on the exact prop

Reviewed changes

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

File Description
src/atoms/types/SideBar.ts Added optional exact property to SideBarMenuItemBase interface with JSDoc documentation
src/organisms/SideBar/MenuItem/MenuItem.utils.ts Exported existing isExactUrl function to make it available for component use
src/organisms/SideBar/MenuItem/BasicMenuItem.tsx Implemented conditional logic to use exact or partial URL matching based on the exact prop
src/organisms/SideBar/MenuItem/CollapsableMenuItem.tsx Updated parent active state determination to respect child items' exact property

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


const isExactUrl = ({ currentUrl, link }: IsUrlArgs) => {
export const isExactUrl = ({ currentUrl, link }: IsUrlArgs) => {
const currentWithoutParams = currentUrl.split('?')[0];
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The isExactUrl function doesn't decode the URL like isCurrentUrl does (line 7), which could lead to inconsistent behavior when comparing encoded vs. non-encoded URLs.

Consider applying the same decoding:

export const isExactUrl = ({ currentUrl, link }: IsUrlArgs) => {
  const decodedCurrentUrl = decodeURIComponent(currentUrl);
  const currentWithoutParams = decodedCurrentUrl.split('?')[0];
  return currentWithoutParams === link;
};
Suggested change
const currentWithoutParams = currentUrl.split('?')[0];
const decodedCurrentUrl = decodeURIComponent(currentUrl);
const currentWithoutParams = decodedCurrentUrl.split('?')[0];

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this is correct, i wasn't sure if reusing the internal isExactUrl was the proper way to do this functionality, unsure if i do this suggestion I might break something. What do you think @mariush2 @aslakihle?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +13 to 15
export const isExactUrl = ({ currentUrl, link }: IsUrlArgs) => {
const currentWithoutParams = currentUrl.split('?')[0];
return currentWithoutParams === link;
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The newly exported isExactUrl function lacks test coverage. Since there are existing tests in MenuItem.utils.test.ts for similar utility functions, consider adding tests for isExactUrl to cover scenarios like:

  • Exact URL match
  • URL with query parameters (should match when params are stripped)
  • URL mismatch
  • Edge cases with trailing slashes

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +46
const isActive = exact
? isExactUrl({
currentUrl: pathname,
link,
})
: isCurrentUrl({
currentUrl: pathname,
link,
});
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The new exact prop behavior in BasicMenuItem lacks test coverage. Since there are comprehensive tests for MenuItem in MenuItem.test.tsx, consider adding tests to verify:

  • Menu item is active only when URL exactly matches (when exact=true)
  • Menu item is active with partial match (when exact=false or undefined)
  • Navigation behavior with exact prop in different URL scenarios

Copilot uses AI. Check for mistakes.
Comment on lines 104 to 108
const isActive = items.some((item) =>
isCurrentUrl({ currentUrl: pathname, link: item.link })
item.exact
? isExactUrl({ currentUrl: pathname, link: item.link })
: isCurrentUrl({ currentUrl: pathname, link: item.link })
);
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The new exact prop behavior in CollapsableMenuItem lacks test coverage. Since there are comprehensive tests for menu items in the test files, consider adding tests to verify:

  • Parent item's active state respects child items' exact property
  • Multiple child items with different exact settings are handled correctly

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Dec 2, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 99.59% (🎯 100%) 16356 / 16422
🔵 Statements 99.59% (🎯 100%) 16356 / 16422
🔵 Functions 99.65% (🎯 100%) 1172 / 1176
🔵 Branches 99.62% (🎯 100%) 4024 / 4039
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/atoms/types/SideBar.ts 100% 100% 100% 100%
src/organisms/SideBar/MenuItem/BasicMenuItem.tsx 95.95% 90% 100% 95.95%
src/organisms/SideBar/MenuItem/CollapsableMenuItem.tsx 99.32% 97.5% 100% 99.32%
src/organisms/SideBar/MenuItem/MenuItem.utils.ts 100% 100% 100% 100%
Generated in workflow #1972 for commit 9bd6c37 by the Vitest Coverage Report Action

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +13 to 16
export const isExactUrl = ({ currentUrl, link }: IsUrlArgs) => {
const currentWithoutParams = currentUrl.split('?')[0];
return currentWithoutParams === link;
};
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The new exact property functionality lacks test coverage. While tests exist for other functionality in this file (e.g., isCurrentUrl, canNavigate), there are no tests for the newly exported isExactUrl function.

Consider adding tests to verify:

  • Exact URL matching works correctly
  • Query parameters are properly stripped before comparison
  • The function handles edge cases (e.g., trailing slashes, encoded characters)

Copilot uses AI. Check for mistakes.
@yunarch
Copy link
Contributor Author

yunarch commented Dec 2, 2025

@mariush2 @aslakihle
A quick note about the existing replace property:

The menu item already includes a replace property, which controls whether a menu item should navigate when the current URL does not match it exactly. In some ways, this partially overlaps with the intention of this PR. However, the property is unintuitive and currently undocumented, which makes its behavior difficult to understand or rely on, but it fixes the problem stated on this PR. However, Regarding the active-state logic, the current code match the assumptions described in this PR, whether intentionally or not. There are actually several valid interpretations of when a sidebar menu item should be considered active:

  • The menu item is active only when the current URL matches its path exactly.

This is the most straightforward interpretation.

For the following two interpretations, it depends on how we want menu items to behave:

  • The menu item is also active when the current URL is a descendant of its path (e.g., the user is navigating deeper into that section).
  • Or the opposite: the menu item is not active when the user is deeper in the hierarchy, because the user is no longer “on” that menu item’s page.

The current implementation effectively assumes only the first one of them. A menu item is also active if the current URL is a descendant of the menu item link.

Given that, perhaps this PR should change and explicitly address this decision point by allowing applications to configure which interpretation they want. At the same time, we may want to clarify and document the intention of the existing replace property so its role is well understood. (maybe even consolidate these 2 problems into 1 property, for when to allow navigation and for when a menu item is active)

@aslakihle
Copy link
Contributor

@yunarch
I will take this up in the next ACL meeting, and we will discuss this. But i agree there at the very least needs to be more clarity JSDoc stuff around the props, that explains them. and most likely a way to control the highlighting aswell 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants