Add breadcrumb primitive - #114
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a breadcrumb component with rendering primitives, accessibility annotations, a demo panel, and gallery/module wiring. Icon rendering also gains a decorative mode that hides glyphs from assistive technologies. ChangesBreadcrumb component and demo
Accessibility support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BreadcrumbDemoPanel
participant BreadcrumbDemo
participant BreadcrumbView
User->>BreadcrumbView: click selectable segment or jump control
BreadcrumbView->>BreadcrumbDemo: update location
BreadcrumbDemoPanel->>BreadcrumbDemo: read updated location during rebuild
BreadcrumbDemoPanel->>BreadcrumbView: regenerate trail view
BreadcrumbDemoPanel-->>User: render updated breadcrumb trail
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/breadcrumb/view.rs`:
- Around line 87-119: Breadcrumb::render is missing an explicit use<Capture> for
the returned opaque widget view, which can cause the elided &Theme lifetime to
be captured and prevent boxing into Box<AnyWidgetView<State, Action>>. Update
Breadcrumb::render to use the suggested use<State, Action> capture on the impl
WidgetView return type so the rendered view stays compatible with the existing
`'static` children collection and button/label/icon rendering path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2957685-a3f3-4bcd-a725-874a20de21bb
📒 Files selected for processing (6)
examples/gallery.rssrc/components/breadcrumb/demo.rssrc/components/breadcrumb/mod.rssrc/components/breadcrumb/view.rssrc/components/mod.rssrc/lib.rs
| pub fn render(self, theme: &Theme) -> impl WidgetView<State, Action> | ||
| where | ||
| State: 'static, | ||
| Action: 'static, | ||
| { | ||
| let mut children: Vec<Box<AnyWidgetView<State, Action>>> = | ||
| Vec::with_capacity(self.segments.len() * 2); | ||
| for (i, seg) in self.segments.into_iter().enumerate() { | ||
| if i > 0 { | ||
| children.push(Box::new( | ||
| icon(IconName::ChevronRight) | ||
| .color(theme.palette.text_faint) | ||
| .size(theme.typography.size_caption) | ||
| .render(theme), | ||
| )); | ||
| } | ||
| let view: Box<AnyWidgetView<State, Action>> = match seg.on_select { | ||
| Some(on_select) => Box::new( | ||
| button(move |s: &mut State| on_select(s)) | ||
| .label(seg.label) | ||
| .variant(ButtonVariant::Text) | ||
| .tint(theme.palette.text_muted) | ||
| .render(theme), | ||
| ), | ||
| None => Box::new(label(seg.label).color(theme.palette.text).render(theme)), | ||
| }; | ||
| children.push(view); | ||
| } | ||
| flex_row(children) | ||
| .cross_axis_alignment(CrossAxisAlignment::Center) | ||
| .gap(Length::px(f64::from(theme.density.pad) / 3.0)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the crate's declared Rust edition and compare `render`/view-builder
# signatures across components for `use<...>` bound consistency.
fd Cargo.toml -x cat {}
echo "--- functions/methods taking &Theme and returning impl WidgetView, with their use<> bounds ---"
rg -n -B2 -A2 'fn render.*theme: &Theme.*-> impl WidgetView' src/components --type=rust
rg -n 'use<' src/components --type=rustRepository: VoidstarSolutions/void_ui
Length of output: 18002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- breadcrumb demo around trail() ---"
sed -n '1,140p' src/components/breadcrumb/demo.rs
echo "--- AnyWidgetView / WidgetView definitions ---"
rg -n -A4 -B4 'type AnyWidgetView|trait WidgetView|impl WidgetView' src -t rustRepository: VoidstarSolutions/void_ui
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- breadcrumb demo state / panel construction ---"
sed -n '1,260p' src/components/breadcrumb/demo.rs
echo "--- breadcrumb view around render() ---"
sed -n '1,180p' src/components/breadcrumb/view.rsRepository: VoidstarSolutions/void_ui
Length of output: 11731
🌐 Web query:
Rust 2024 RPIT precise capturing use<...> anonymous lifetime &Theme trait object AnyWidgetView 'static
💡 Result:
In the Rust 2024 edition, return-position impl Trait (RPIT) opaque types automatically capture all in-scope type and lifetime parameters [1][2]. Precise capturing, enabled via the use<...> syntax, allows developers to explicitly control or "opt-out" of this default behavior by specifying exactly which parameters are captured by the opaque type [3][4]. The use<...> syntax functions as a syntactic bound [5]. For example, impl Trait + use<'a> specifies that only the lifetime 'a is captured [6]. Using use<> (empty brackets) is a common way to specify that the opaque type should capture nothing, which can be useful for ensuring an opaque type is 'static when it would otherwise implicitly capture a non-'static lifetime [7]. Regarding AnyWidgetView in the Xilem framework, it is a type alias representing a trait object: dyn AnyView<State, Action, ViewCtx, Pod<DynWidget>> + Send + Sync [8][9]. Because it is a dynamic trait object, it does not involve RPIT opaque types directly in its definition. However, when working with WidgetView trait methods such as boxed()—which returns a Box<AnyWidgetView<...>>—you may encounter use<...> bounds in function signatures to manage capture requirements for the underlying types being returned [10][11]. For instance, a function returning an impl WidgetView<State> may be annotated with + use<State> to explicitly capture the State type parameter, satisfying the requirements for the returned opaque type [11]. In summary: - Rust 2024 automatically captures all in-scope generics in RPIT [2]. - use<...> provides granular control to limit or precisely define these captures [3][6]. - AnyWidgetView is a dyn-compatible type alias used in Xilem for type-erased views [8][12]. - The use<...> syntax is often used in conjunction with trait-based return types to ensure the correct generic parameters (like State) are captured for the opaque return type [11].
Citations:
- 1: https://doc.rust-lang.org/edition-guide/rust-2024/rpit-lifetime-capture.html
- 2: https://rust-lang.github.io/rfcs/3498-lifetime-capture-rules-2024.html
- 3: https://doc.rust-lang.org/std/keyword.use.html
- 4: https://doc.rust-lang.org/stable/std/keyword.use.html
- 5: Stabilize opaque type precise capturing (RFC 3617) rust-lang/rust#127672
- 6: https://stackoverflow.com/questions/79129687/what-is-rusts-uselifetime-syntax
- 7: https://users.rust-lang.org/t/understanding-proper-usage-of-precise-capturing-in-the-2024-edition/116587
- 8: https://docs.rs/xilem/latest/xilem/type.AnyWidgetView.html
- 9: https://docs.rs/xilem/latest/src/xilem/any_view.rs.html
- 10: https://docs.rs/xilem/latest/xilem/trait.WidgetView.html
- 11: https://docs.rs/xilem/latest/src/xilem/widget_view.rs.html
- 12: https://docs.rs/xilem_core/latest/xilem_core/trait.AnyView.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- AnyWidgetView alias and boxing helpers ---"
rg -n -A6 -B6 'type AnyWidgetView|fn boxed|boxed\(' src /root/.cargo/registry -g '*.rs' 2>/dev/null | head -n 200Repository: VoidstarSolutions/void_ui
Length of output: 8204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- boxing patterns for AnyWidgetView ---"
rg -n -A3 -B3 'Box<AnyWidgetView|boxed\(\)|AnyWidgetView<' src -g '*.rs'Repository: VoidstarSolutions/void_ui
Length of output: 42377
Add + use<State, Action> to Breadcrumb::render. In edition 2024, the elided &Theme lifetime is captured by the opaque return type, which makes this view harder to box into the 'static Box<AnyWidgetView<...>> used by the breadcrumb demo/state path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/breadcrumb/view.rs` around lines 87 - 119, Breadcrumb::render
is missing an explicit use<Capture> for the returned opaque widget view, which
can cause the elided &Theme lifetime to be captured and prevent boxing into
Box<AnyWidgetView<State, Action>>. Update Breadcrumb::render to use the
suggested use<State, Action> capture on the impl WidgetView return type so the
rendered view stays compatible with the existing `'static` children collection
and button/label/icon rendering path.
e464b8a to
9e749a5
Compare
17b70e8 to
ec3e6b1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/access_wrap.rs (2)
195-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: tests only cover
accessibility_role, not the node mutation. Theaccessibility()branches (set_aria_current(Page)/set_hidden()) — the actual behavior callers depend on — are untested. Worth adding if aNodecan be constructed in a unit test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/access_wrap.rs` around lines 195 - 225, The current tests in AccessAnnotateWidget only verify accessibility_role(), but they do not cover the behavior in accessibility() where the Node is mutated for AccessAnnotation::CurrentPage and AccessAnnotation::Hidden. Add a unit test around AccessAnnotateWidget::accessibility() that constructs or mocks a Node and asserts the branches call set_aria_current(Page) and set_hidden() respectively, alongside the existing role checks, so the actual accessibility mutations are covered.
170-188: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an accessible name to the navigation landmark. The breadcrumb wrapper is exposed as
Role::Navigationwith no label; threading an optional label throughannotate(for example,node.set_label("Breadcrumb")) would make it easier to distinguish from other navigation landmarks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/access_wrap.rs` around lines 170 - 188, The breadcrumb wrapper’s Navigation landmark is unlabeled, so update access_wrap.rs to thread an optional label through annotate/accessibility for AccessAnnotation::Navigation and set an accessible name on the node (for example via node.set_label) while keeping Role::Navigation unchanged; use the accessibility_role and accessibility methods in AccessWrap to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/access_wrap.rs`:
- Around line 195-225: The current tests in AccessAnnotateWidget only verify
accessibility_role(), but they do not cover the behavior in accessibility()
where the Node is mutated for AccessAnnotation::CurrentPage and
AccessAnnotation::Hidden. Add a unit test around
AccessAnnotateWidget::accessibility() that constructs or mocks a Node and
asserts the branches call set_aria_current(Page) and set_hidden() respectively,
alongside the existing role checks, so the actual accessibility mutations are
covered.
- Around line 170-188: The breadcrumb wrapper’s Navigation landmark is
unlabeled, so update access_wrap.rs to thread an optional label through
annotate/accessibility for AccessAnnotation::Navigation and set an accessible
name on the node (for example via node.set_label) while keeping Role::Navigation
unchanged; use the accessibility_role and accessibility methods in AccessWrap to
locate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc61dd7b-fbd0-4251-a898-8d38f627b6d9
📒 Files selected for processing (8)
examples/gallery.rssrc/components/access_wrap.rssrc/components/breadcrumb/demo.rssrc/components/breadcrumb/mod.rssrc/components/breadcrumb/view.rssrc/components/icon/view.rssrc/components/mod.rssrc/lib.rs
✅ Files skipped from review due to trivial changes (2)
- src/lib.rs
- examples/gallery.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/breadcrumb/mod.rs
- src/components/mod.rs
- src/components/breadcrumb/view.rs
- src/components/breadcrumb/demo.rs
b03a31f to
605b10c
Compare
An ordered trail of segments joined by a themed chevron separator, for app-chrome navigation (CITADEL › Trade dashboard). Pure composition, no custom widget: flex_row of button/label/icon. A segment is interactive (renders as a quiet inline button) if built with .on_select(...), or a plain current-location label otherwise — so "last segment styled as current by default" falls out naturally from simply not attaching a callback to it, with no separate "is current" flag to keep in sync. Closes #105.
New src/components/breadcrumb/widget.rs: BreadcrumbNav, a transparent single-child pass-through widget (same shape as masonry's own Passthrough/SizedBox) whose only job is accessibility_role() -> Role::Navigation. No layout/paint of its own — it delegates measure/layout/children straight through to its child. view.rs: added BreadcrumbNavView<V>, a thin hand-written View wrapper (mirroring xilem_masonry's own sized_box implementation) that builds the flex_row trail, then wraps its resulting widget in BreadcrumbNav. Breadcrumb::render now returns this wrapped view instead of the bare flex_row. The trick that made this tractable: rather than fighting AnyWidgetView's type-erasure (which is hardcoded to Pod<Passthrough> inside xilem itself, so you can't substitute a custom role there), the wrapper stores its child as a plain masonry WidgetPod<dyn Widget> — exactly how masonry's own SizedBox/Passthrough do it — and uses .downcast() in rebuild/teardown/message to get back to the concrete inner view's element type.
BreadcrumbCurrent (new widget in breadcrumb/widget.rs) — same transparent single-child pass-through shape as BreadcrumbNav, but its accessibility() calls node.set_aria_current(AriaCurrent::Page) — accesskit's native equivalent of the web's aria-current="page". Reports Role::GenericContainer itself (the text stays exposed via the wrapped label). BreadcrumbCurrentView<V> (new in view.rs) — same hand-written single-child View pattern as BreadcrumbNavView, wrapping just the trailing/current segment's label. Breadcrumb::render's "no on_select" branch now wraps its label(...) in this instead of rendering it bare.
New shared primitive: src/components/access_wrap.rs — since this was the third time I needed "a transparent single-child widget that exists only to attach one piece of accesskit state" (after BreadcrumbNav for #1 and BreadcrumbCurrent for #2), I consolidated all three into one AccessAnnotateWidget + annotate() view function, parameterized by an AccessAnnotation enum (Navigation / CurrentPage / Hidden). This replaced breadcrumb's two bespoke widgets (deleted breadcrumb/widget.rs entirely) and is now the crate-wide fix point for this whole class of problem. icon().decorative() — new builder method. When set, wraps the rendered glyph in annotate(_, AccessAnnotation::Hidden), which calls accesskit's node.set_hidden(). This is the actual crate-wide fix: any icon anywhere in void_ui can now opt out of being read by a screen reader, not just breadcrumb's. Breadcrumb's chevron separator now calls .decorative(), closing the specific gap you flagged.
605b10c to
7138cc1
Compare
1 participant
An ordered trail of segments joined by a themed chevron separator, for app-chrome navigation (CITADEL › Trade dashboard). Pure composition, no custom widget: flex_row of button/label/icon.
A segment is interactive (renders as a quiet inline button) if built with .on_select(...), or a plain current-location label otherwise — so "last segment styled as current by default" falls out naturally from simply not attaching a callback to it, with no separate "is current" flag to keep in sync.
Closes #105.
Summary by CodeRabbit