Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
Greptile SummaryThis PR fixes broken parent breadcrumb links in the docs site — specifically for routes like
Confidence Score: 4/5Safe to merge for the primary Enterprise breadcrumb fix; the fallback branch returns a non-trailing-slash URL which could cause navigation inconsistencies for routes not in the registry. The breadcrumb resolver correctly handles the registered-route and overview-fallback cases. The last return hands back the raw, un-normalized path rather than the already-computed The fallback return in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["@docpage decorator applied\n(import time)"] --> B["_register_doc_route(path)"]
B --> C["_REGISTERED_DOC_ROUTES.add(\n_normalize_doc_route(path)\n)"]
D["breadcrumb(path, …) called\n(request time)"] --> E["Split path into segments\n(strip 'docs')"]
E --> F["Build current_path\nfor each segment"]
F --> G["_resolve_breadcrumb_href(current_path)"]
G --> H{"_normalize_doc_route(href)\nin routes?"}
H -- Yes --> I["Return normalized route"]
H -- No --> J{"_normalize_doc_route(\nhref + 'overview')\nin routes?"}
J -- Yes --> K["Return overview route\ne.g. /enterprise/overview/"]
J -- No --> L["Return raw href ⚠️\n(not normalized)"]
I & K & L --> M["rx.el.a href=…"]
Reviews (1): Last reviewed commit: "Fix docs breadcrumb overview routes" | Re-trigger Greptile |
| overview_route = _normalize_doc_route(f"{route}overview") | ||
| if overview_route in routes: | ||
| return overview_route | ||
|
|
||
| return href |
There was a problem hiding this comment.
The fallback branch returns the raw, un-normalized
href (e.g. /enterprise with no trailing slash) while every other return in this function returns the fully-normalized route (e.g. /enterprise/). When a path segment has no matching route and no …/overview/ variant, the resulting breadcrumb link will be missing a trailing slash, producing an inconsistent URL relative to all other crumbs. Returning route keeps the output uniform.
| overview_route = _normalize_doc_route(f"{route}overview") | |
| if overview_route in routes: | |
| return overview_route | |
| return href | |
| overview_route = _normalize_doc_route(f"{route}overview") | |
| if overview_route in routes: | |
| return overview_route | |
| return route |
| rendered = str( | ||
| docpage_module.breadcrumb("/enterprise/ag-grid/pivot-mode/", rx.box()) | ||
| ) | ||
|
|
||
| assert 'to:"/enterprise/overview/"' in rendered | ||
| assert 'to:"/enterprise/ag-grid/"' in rendered | ||
| assert 'to:"/enterprise/ag-grid/pivot-mode/"' in rendered |
There was a problem hiding this comment.
Fragile serialization-based assertions
The assertions match against Reflex's internal component serialization string (e.g. 'to:"/enterprise/overview/"'). If Reflex ever changes how it stringifies component props — which is an implementation detail, not a public contract — these assertions will silently break even when the breadcrumb logic is perfectly correct. A less brittle alternative would be to call _resolve_breadcrumb_href directly for the intermediate path segments, or to inspect the rendered component tree's props rather than the raw str() output.
Summary
/overview/when the synthesized parent path is not a route.Root Cause
The docs breadcrumb builder created parent links by concatenating path segments. For Enterprise docs, that produced
/docs/enterprise/, but the real route is/docs/enterprise/overview/.Validation
uv run pytest tests/test_breadcrumbs.py tests/test_routes.py -quv run ruff check reflex_docs/templates/docpage/docpage.py tests/test_breadcrumbs.py/docs/enterprise/overview/.Fixes #6437