feat: add breadcrumbs and superposition platform links#970
feat: add breadcrumbs and superposition platform links#970
Conversation
|
Review changes with Changed Files
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA breadcrumb navigation component is introduced to the frontend, automatically deriving breadcrumb segments from URL paths and organizational context. The component is integrated into the layout, the side navigation's root link is adjusted, and backend redirect routes are added for URL consistency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a global breadcrumb trail to the Superposition UI to improve navigation across deeply nested admin/workspace pages, and adjusts platform entry links/redirects to support the new navigation flow.
Changes:
- Added Actix redirects for org/workspace “root” admin paths to land on workspaces/default-config pages.
- Introduced a new
Breadcrumbscomponent and injected it into the sharedCommonLayout. - Updated the SideNav “Superposition Platform” link to point to
/admin/organisations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/superposition/src/main.rs | Adds redirects for /admin/{org_id} and /admin/{org_id}/{tenant} paths. |
| crates/frontend/src/types.rs | Introduces BreadcrumbSegment type for breadcrumb rendering. |
| crates/frontend/src/hoc/layout.rs | Renders the new breadcrumbs in CommonLayout. |
| crates/frontend/src/components/side_nav.rs | Updates platform link target to organisations page. |
| crates/frontend/src/components/breadcrumbs.rs | Implements breadcrumb parsing and UI rendering. |
| crates/frontend/src/components.rs | Exposes the new breadcrumbs component module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Builds the breadcrumb trail from the current URL path. | ||
| fn build_breadcrumbs(path: &str, base: &str) -> Vec<BreadcrumbSegment> { | ||
| // Strip the base prefix if present | ||
| let path = path.replace(base, ""); |
There was a problem hiding this comment.
let path = path.replace(base, ""); removes all occurrences of base anywhere in the path, not just a leading prefix. If base ever appears later in the URL (or is a short string), this can corrupt the breadcrumb parsing. Prefer stripping only a leading prefix (e.g. a strip_prefix-style approach) and leaving the rest of the path intact.
| let path = path.replace(base, ""); | |
| let path = path.strip_prefix(base).unwrap_or(path); |
| const SKIP_SEGMENTS: [&str; 4] = ["admin", "action", "authz", "org-authz"]; | ||
|
|
||
| /// Maps a URL path segment to a human-readable label. | ||
| /// Returns None for segments that should be skipped (like "action"). |
There was a problem hiding this comment.
The doc comment for segment_to_label says it "Returns None for segments that should be skipped (like "action")", but this function currently always returns Some(...) for any input. Either update the doc to reflect that skipping is handled elsewhere, or implement the skip logic inside segment_to_label and have callers rely on None.
| /// Returns None for segments that should be skipped (like "action"). | |
| /// Segment skipping is handled by the caller (for example, `build_breadcrumbs`). |
| .service(web::redirect("/admin/{org_id}", "workspaces")) | ||
| .service(web::redirect("/admin/{org_id}/", "workspaces")) | ||
| .service(web::redirect("/admin/{org_id}/{tenant}", "default-config")) | ||
| .service(web::redirect("/admin/{org_id}/{tenant}/", "default-config")) |
There was a problem hiding this comment.
The redirects for /admin/{org_id} and /admin/{org_id}/{tenant} use relative Location values ("workspaces" / "default-config"). When the incoming URL does not end with a trailing slash (e.g. /admin/123), browsers resolve relative redirects by replacing the last path segment, which can drop {org_id} / {tenant} (e.g. /admin/workspaces). Use an absolute redirect target that preserves the captured params (e.g. construct the full path in a handler), or redirect /admin/{org_id} -> /admin/{org_id}/ first and only then redirect the trailing-slash route to the subpage.
| let current = if previous_segments == "/admin" { | ||
| "organisations" | ||
| } else if previous_segments | ||
| .split("/") | ||
| .filter(|s| !s.is_empty()) | ||
| .collect::<Vec<&str>>() | ||
| .len() | ||
| == 2 | ||
| { | ||
| "workspaces" | ||
| } else { | ||
| segment | ||
| }; | ||
| format!("{}{}/{}", base, previous_segments, current) | ||
| }, |
There was a problem hiding this comment.
build_breadcrumbs rewrites the target href for some crumbs based on previous_segments (mapping to "organisations" / "workspaces"). This makes breadcrumb labels (e.g. an org/workspace id) link to a different page than the path segment they represent, and it can prevent users from navigating back to the actual intermediate URL segments. Consider generating each crumb href as the progressively accumulated path up to that segment (relying on the server redirects for /admin/{org_id} and /admin/{org_id}/{workspace}), instead of substituting fixed segments here.
Signed-off-by: datron <Datron@users.noreply.github.com>
Datron
force-pushed
the
improve-navigation
branch
from
cd29cb9 to
0b86cca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/frontend/src/components/breadcrumbs.rs (2)
68-81: Consider extracting the segment-counting logic for clarity.The condition at lines 70-75 counts non-empty path segments to determine if we're at the organization level. While functional, this could be more readable.
♻️ Optional: Extract segment count helper
+fn count_path_segments(path: &str) -> usize { + path.split('/').filter(|s| !s.is_empty()).count() +} + fn build_breadcrumbs(path: &str, base: &str) -> Vec<BreadcrumbSegment> { // ... existing code ... } else { let current = if previous_segments == "/admin" { "organisations" - } else if previous_segments - .split("/") - .filter(|s| !s.is_empty()) - .collect::<Vec<&str>>() - .len() - == 2 - { + } else if count_path_segments(&previous_segments) == 2 { + // At org level: /admin/{org_id} "workspaces" } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/breadcrumbs.rs` around lines 68 - 81, The inline logic in breadcrumbs.rs that counts non-empty path segments (used in the if that sets current based on previous_segments) is hard to read; extract it into a small helper like a private function (e.g., non_empty_segment_count or count_segments) that accepts &str and returns usize, then replace the long chain previous_segments.split(...).filter(...).collect::<Vec<&str>>().len() == 2 with a call to that helper (e.g., count_segments(previous_segments) == 2) and use that helper when computing current and the format! call so the intent around previous_segments and current is clearer.
52-55: Minor: Consider usingstrip_prefixfor safer base removal.
String::replacewill replace all occurrences ofbasein the path. If the base prefix happens to appear again in a segment name (unlikely but possible), it could cause unexpected behavior. Usingstrip_prefixis more precise for this use case.♻️ Suggested improvement
fn build_breadcrumbs(path: &str, base: &str) -> Vec<BreadcrumbSegment> { // Strip the base prefix if present - let path = path.replace(base, ""); + let path = path.strip_prefix(base).unwrap_or(path); let segments: Vec<&str> = path.split('/').filter(|s| !s.is_empty()).collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/frontend/src/components/breadcrumbs.rs` around lines 52 - 55, The current build_breadcrumbs function uses path.replace(base, "") which may remove non-prefix occurrences; change it to use strip_prefix for precise removal: in build_breadcrumbs(&str, base: &str) call path.strip_prefix(base) and if it returns Some(stripped) use that stripped &str, otherwise keep the original path, then continue splitting/filtering into segments; update the local binding that currently holds the replaced path accordingly so types remain &str and downstream code (e.g., segments creation) is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/frontend/src/components/breadcrumbs.rs`:
- Around line 68-81: The inline logic in breadcrumbs.rs that counts non-empty
path segments (used in the if that sets current based on previous_segments) is
hard to read; extract it into a small helper like a private function (e.g.,
non_empty_segment_count or count_segments) that accepts &str and returns usize,
then replace the long chain
previous_segments.split(...).filter(...).collect::<Vec<&str>>().len() == 2 with
a call to that helper (e.g., count_segments(previous_segments) == 2) and use
that helper when computing current and the format! call so the intent around
previous_segments and current is clearer.
- Around line 52-55: The current build_breadcrumbs function uses
path.replace(base, "") which may remove non-prefix occurrences; change it to use
strip_prefix for precise removal: in build_breadcrumbs(&str, base: &str) call
path.strip_prefix(base) and if it returns Some(stripped) use that stripped &str,
otherwise keep the original path, then continue splitting/filtering into
segments; update the local binding that currently holds the replaced path
accordingly so types remain &str and downstream code (e.g., segments creation)
is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d6281eb-1d13-4717-a3bf-0c75ca494b99
📒 Files selected for processing (6)
crates/frontend/src/components.rscrates/frontend/src/components/breadcrumbs.rscrates/frontend/src/components/side_nav.rscrates/frontend/src/hoc/layout.rscrates/frontend/src/types.rscrates/superposition/src/main.rs
| "types", | ||
| "audit-log", | ||
| "default-config", | ||
| "experiment-groups", | ||
| "compare", | ||
| "config", | ||
| "dimensions", | ||
| "experiments", | ||
| "function", | ||
| "resolve", | ||
| "secrets", | ||
| "variables", | ||
| "versions", | ||
| "webhooks", | ||
| "workspaces", | ||
| "overrides", | ||
| "create", | ||
| "edit", |
There was a problem hiding this comment.
do we need this list separately ?
There was a problem hiding this comment.
@ayushjain17 looked into unifying this array string and Resource enum, there are a couple of differences:
- Resource uses snake_case but all our url segments are kebab-case. I can't change resource I guess because of casbin DB
- Resource doesn't have some of the title segments I am looking for like Create and Edit, resolve
- Resource is mostly singular, but all our title segments are plural, eg:
Dimensionin Resource and dimensions in the URL
Writing code to sync these two would be more work and still fragile. I am thinking of changing app.rs to instead use a different Enum for the path names like default-config, dimensions, etc which can then convert to Resource type if needed. What do you think?
Problem
Navigation backwards is hard in superposition if deeply nested
Solution
Add breadcrumbs so users can see where they are and jump to any previous screen they had visited along the way to the current page
Screen.Recording.2026-04-14.at.11.55.05.AM.mov
Summary by CodeRabbit
New Features
Improvements