Repository navigation
fix(webapp): retain sidebar item active highlight on nested detail pages #5097
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,93 @@ export function SideMenuLabel({ | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Normalizes a URL path by stripping any hash, query parameters, and trailing slashes. | ||
| */ | ||
| export function normalizePath(path: string): string { | ||
| const [clean = ""] = path.split("#")[0].split("?"); | ||
| if (clean.length > 1 && clean.endsWith("/")) { | ||
| return clean.slice(0, -1); | ||
| } | ||
| return clean; | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether currentPath matches targetPath, optionally checking nested paths | ||
| * while respecting segment boundaries. | ||
| */ | ||
| export function matchesPath(currentPath: string, targetPath: string, matchPrefix = false): boolean { | ||
| const normalizedCurrent = normalizePath(currentPath); | ||
| const normalizedTarget = normalizePath(targetPath); | ||
|
|
||
| if (normalizedCurrent === normalizedTarget) { | ||
| return true; | ||
| } | ||
|
|
||
| if (matchPrefix) { | ||
| return normalizedCurrent.startsWith(`${normalizedTarget}/`); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export type IsSideMenuItemActiveOptions = { | ||
| currentPath: string; | ||
| targetPath: string; | ||
| matchPrefix?: boolean; | ||
| nestedPaths?: string[]; | ||
| isActiveOverride?: boolean; | ||
| yieldActiveToFavorite?: boolean; | ||
| activeFavoriteId?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Determines whether a sidebar navigation item should be marked active. | ||
| * | ||
| * Respects: | ||
| * - Direct isActive overrides (e.g. from favorites). | ||
| * - Yielding active highlight to a matching active favorite. | ||
| * - Exact path matching. | ||
| * - Segment-boundary-aware prefix matching (e.g. /concurrency does NOT match /concurrency-limits). | ||
| * - Nested paths for parent items whose subpages live under distinct paths (e.g. Tasks). | ||
| */ | ||
| export function isSideMenuItemActive({ | ||
| currentPath, | ||
| targetPath, | ||
| matchPrefix = false, | ||
| nestedPaths, | ||
| isActiveOverride, | ||
| yieldActiveToFavorite = false, | ||
| activeFavoriteId, | ||
| }: IsSideMenuItemActiveOptions): boolean { | ||
| if (isActiveOverride !== undefined) { | ||
| return isActiveOverride; | ||
| } | ||
|
|
||
| if (yieldActiveToFavorite && activeFavoriteId !== undefined) { | ||
| return false; | ||
| } | ||
|
|
||
| if (matchesPath(currentPath, targetPath, matchPrefix)) { | ||
| return true; | ||
| } | ||
|
|
||
| if (nestedPaths && nestedPaths.length > 0) { | ||
| const normalizedTarget = normalizePath(targetPath); | ||
| for (const nested of nestedPaths) { | ||
| const resolved = | ||
| nested.startsWith("/") && !nested.startsWith(normalizedTarget) | ||
| ? `${normalizedTarget}${nested}` | ||
| : nested; | ||
| if (matchesPath(currentPath, resolved, true)) { | ||
|
Comment on lines
+138
to
+142
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Clarify nested path input semantics The Tasks caller supplies full paths under its target, so they resolve as intended. A full path outside that target instead gets concatenated to it. Define whether Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export function SideMenuItem({ | ||
| icon, | ||
| activeIconColor, | ||
|
|
@@ -80,6 +167,7 @@ export function SideMenuItem({ | |
| isActive: isActiveOverride, | ||
| yieldActiveToFavorite = false, | ||
| matchPrefix = false, | ||
| nestedPaths, | ||
| "data-action": dataAction, | ||
| }: { | ||
| icon?: RenderIcon; | ||
|
|
@@ -101,6 +189,8 @@ export function SideMenuItem({ | |
| /** Overrides the default pathname === to active check (e.g. favorites match on full URL). */ | ||
| isActive?: boolean; | ||
| matchPrefix?: boolean; | ||
| /** Additional subpaths (e.g. /tasks, /agents) that should activate this parent item. */ | ||
| nestedPaths?: string[]; | ||
| /** | ||
| * In menus that render the Favorites section (the main project menu), an active favorite owns | ||
| * the highlight, so the plain item yields its active state to it. Menus without a favorites | ||
|
|
@@ -114,10 +204,15 @@ export function SideMenuItem({ | |
| // Only the user's OWN favorites own a view (via the marker param); markers from shared links | ||
| // don't count (see useActiveFavoriteId). | ||
| const activeFavoriteId = useActiveFavoriteId(); | ||
| const isActive = | ||
| isActiveOverride ?? | ||
| ((pathName === to || (matchPrefix && pathName.startsWith(`${to}/`))) && | ||
| (!yieldActiveToFavorite || activeFavoriteId === undefined)); | ||
| const isActive = isSideMenuItemActive({ | ||
| currentPath: pathName, | ||
| targetPath: to, | ||
| matchPrefix, | ||
| nestedPaths, | ||
| isActiveOverride, | ||
| yieldActiveToFavorite, | ||
| activeFavoriteId, | ||
| }); | ||
|
|
||
| const isIndented = indented && !isCollapsed; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Sidebar fix absent from release notes
When this webapp-only sidebar fix ships, users receive no release note for it. The required
.server-changes/entry is missing.Learn more
The repository requires a
.server-changes/entry for a user-facing fix confined to server apps (release-note rule). This change makes sidebar highlights persist on nested pages, but the PR adds no release note. Customers therefore cannot discover the fix in the release summary.Example: A user who previously lost the Runs highlight on a run detail page receives the fix, but sees no entry announcing it.
Recommended fix: Add a new
.server-changes/Markdown file witharea: webapp,type: fix, and one sentence describing the sidebar behavior for users.Was this helpful? React with 👍 or 👎 to provide feedback.