Repository navigation
Conversation
Fixes the issue where sidebar navigation items lost active highlight when navigating from section list pages to nested detail pages (e.g. Tasks and Runs). Supports nestedPaths for Tasks, boundary-safe prefix matching, and preserves dashboard and favorite isolation.
|
Caution Review failedThe pull request is closed.
Tip 🎯 Customize your risk assessments
✅ Automatic approvals, with your rules
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
|
|
Hi @Ashik9500, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
| nestedPaths={[ | ||
| `${v3EnvironmentPath(organization, project, environment)}/tasks`, | ||
| `${v3EnvironmentPath(organization, project, environment)}/agents`, | ||
| ]} |
There was a problem hiding this comment.
🟡 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 with area: webapp, type: fix, and one sentence describing the sidebar behavior for users.
Was this helpful? React with 👍 or 👎 to provide feedback.
| normalizePath, | ||
| } from "~/components/navigation/SideMenuItem"; | ||
|
|
||
| describe("Sidebar Navigation Route Matching", () => { |
There was a problem hiding this comment.
🟡 Unit tests lack required exception rationale
When sideMenuNavigation adds unit tests for route-matching helpers, it omits the required one-line comment explaining the exception. The repository's test rules require that rationale for new unit tests.
Learn more
The repository's testing rules permit new unit tests only for complex, stable logic unlikely to change and require a one-line comment explaining the exception (testing rule). This suite tests helpers in isolation but includes no explanation. Its many repetitive cases also make it harder to show why unit-level coverage is necessary here.
Example: A reviewer reaches the new route-matching suite and cannot tell why its pure helper tests qualify for the unit-test exception.
Recommended fix: Prefer a concise integration test of actual sidebar behavior; if the route matcher merits unit tests, add the required one-line explanation and trim redundant cases.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const resolved = | ||
| nested.startsWith("/") && !nested.startsWith(normalizedTarget) | ||
| ? `${normalizedTarget}${nested}` | ||
| : nested; | ||
| if (matchesPath(currentPath, resolved, true)) { |
There was a problem hiding this comment.
🔍 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 nestedPaths accepts relative paths, absolute paths, or both.
Was this helpful? React with 👍 or 👎 to provide feedback.
| it("highlights Runs on nested run streaming and span subpages", () => { | ||
| expect( | ||
| isSideMenuItemActive({ | ||
| currentPath: `${envPath}/runs/run_cm123456789/stream`, | ||
| ...runsItemConfig, | ||
| }) | ||
| ).toBe(true); | ||
|
|
||
| expect( | ||
| isSideMenuItemActive({ | ||
| currentPath: `${envPath}/runs/run_cm123456789/spans/span_001`, | ||
| ...runsItemConfig, | ||
| }) |
There was a problem hiding this comment.
🔍 Some route tests use invented subpaths
The run span link uses a span query parameter, not the tested /spans/:id path. The custom dashboard widget subpath also lacks a matching page route. These assertions cover prefix matching rather than those user journeys.
Was this helpful? React with 👍 or 👎 to provide feedback.
Fixes the issue where sidebar navigation items lost active highlight when navigating from section list pages to nested detail pages (e.g. Tasks and Runs). Supports nestedPaths for Tasks, boundary-safe prefix matching, and preserves dashboard and favorite isolation.
Closes #
✅ Checklist
Testing
[Describe the steps you took to test this change]
Changelog
[Short description of what has changed]
Screenshots
[Screenshots]
💯