feat: DH-21898: Local routing#1348
Conversation
There was a problem hiding this comment.
Pull request overview
Adds local (widget-scoped) routing support to the Deephaven UI plugin by synchronizing richer URL state between the browser and backend, introducing new routing hooks/components, and documenting/testing the new API surface.
Changes:
- Extend URL state syncing (path, absolute path, fragment, href, plus query params) and make navigation-triggered URL updates propagate to the backend.
- Add Python routing primitives:
use_path,use_navigate,use_params,use_url_components, plusroute/routercomponents andlink(to=...)SPA navigation. - Add unit + e2e coverage and documentation pages for the new routing features.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ui_routing.spec.ts | New Playwright e2e coverage for routing hooks/components. |
| tests/app.d/ui_routing.py | Adds UI panels used by routing e2e tests (and examples for hooks/components). |
| plugins/ui/test/deephaven/ui/test_utils_root.py | Extends test root to carry additional URL fields. |
| plugins/ui/test/deephaven/ui/test_routing.py | Adds Python unit tests for routing hooks and router matching behavior. |
| plugins/ui/src/js/src/widget/WidgetUtils.tsx | Moves Link import to the plugin elements export location. |
| plugins/ui/src/js/src/widget/WidgetHandler.tsx | Captures/sends extended URL state; provides navigate callback via context; listens for URL change events. |
| plugins/ui/src/js/src/widget/WidgetHandler.test.tsx | Updates tests for extended URL state + navigation behavior. |
| plugins/ui/src/js/src/events/NavigateContext.ts | New context to provide widget-scoped navigation callback to children. |
| plugins/ui/src/js/src/events/Navigate.ts | Enhances Navigate to support path/fragment + emits URL-changed event; exports URL state param keys. |
| plugins/ui/src/js/src/events/Navigate.test.ts | Adds unit tests for Navigate and widget-relative path parsing. |
| plugins/ui/src/js/src/elements/Link.tsx | New frontend Link element that can invoke SPA navigation via NavigateContext. |
| plugins/ui/src/js/src/elements/index.ts | Exposes new Link element export. |
| plugins/ui/src/deephaven/ui/types/types.py | Adds NavigationTarget type; documents QueryParams usage. |
| plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py | Stores and updates extended URL state fields; applies URL-state updates from client. |
| plugins/ui/src/deephaven/ui/hooks/use_url_components.py | New hook to access split URL components from href. |
| plugins/ui/src/deephaven/ui/hooks/use_path.py | New hook to access widget-relative vs absolute path. |
| plugins/ui/src/deephaven/ui/hooks/use_params.py | New hook to access matched router params via context. |
| plugins/ui/src/deephaven/ui/hooks/use_navigate.py | New hook to send navigate events (path/query/fragment/replace) to frontend. |
| plugins/ui/src/deephaven/ui/hooks/init.py | Exports new routing hooks. |
| plugins/ui/src/deephaven/ui/components/router.py | New router implementation (compile/match routes, provide params context). |
| plugins/ui/src/deephaven/ui/components/route.py | New route() factory / _Route dataclass. |
| plugins/ui/src/deephaven/ui/components/link.py | Adds to= SPA navigation support for ui.link. |
| plugins/ui/src/deephaven/ui/components/init.py | Exports new route and router components. |
| plugins/ui/src/deephaven/ui/_internal/RootRenderContextProtocol.py | Extends protocol with path/fragment/href accessors. |
| plugins/ui/src/deephaven/ui/_internal/RenderContext.py | Imports and exposes extended URL state fields. |
| plugins/ui/docs/sidebar.json | Adds docs entries for new routing hooks/components. |
| plugins/ui/docs/hooks/use_url_components.md | New docs page for use_url_components. |
| plugins/ui/docs/hooks/use_path.md | New docs page for use_path. |
| plugins/ui/docs/hooks/use_params.md | New docs page for use_params. |
| plugins/ui/docs/hooks/use_navigate.md | New docs page for use_navigate. |
| plugins/ui/docs/components/router.md | New docs page for router/route. |
| plugins/ui/docs/components/link.md | Updates link docs to include SPA navigation via to. |
| .github/skills/writing-docs/SKILL.md | Adds internal documentation-writing guidance for contributors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 1. Static segments are preferred over parameterized segments. | ||
| 2. Longer matches (more segments) are preferred over shorter ones. | ||
| 3. Wildcard routes (`*`) have the lowest priority. | ||
| 4. Optional segments are matched greedily. |
There was a problem hiding this comment.
I assume "greedily" will be a familiar term to your target audience?
There was a problem hiding this comment.
Generally yes, but there's a clearer way to phrase it anyways.
margaretkennedy
left a comment
There was a problem hiding this comment.
Docs look good, minor comments
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
|
ui docs preview (Available for 14 days) |
| logger.debug("Setting URL state: %s", url_state) | ||
| self.set_query_params(url_state.get("__queryParams", {})) | ||
| if "__queryParams" in url_state: | ||
| self.set_query_params(url_state["__queryParams"]) |
There was a problem hiding this comment.
_UrlState should have all these parameter types I guess?
Actually why are we passing a whole dictionary of the parsed URL over the wire, and then storing it as separate parts here? I think we should just pass the URL as a string over the wire, and then we can parse out the components from that. Then we only need to store the URL, and it's consistent (e.g. absolute_path and path wouldn't get out of sync)
| """ | ||
| return self._root.get_query_params() | ||
|
|
||
| def get_path(self) -> str: |
There was a problem hiding this comment.
Ditto - we should just be storing this all as the whole URL, and parse from the same source.
| @@ -35,3 +35,35 @@ def get_query_params(self) -> QueryParams: | |||
| def set_query_params(self, query_params: QueryParams) -> None: | |||
| """Update the URL query parameters.""" | |||
| ... | |||
There was a problem hiding this comment.
We should just have a get_url/set_url methods, instead of all these separate methods. Then in use_navigate and all the other hooks, we can parse/mutate as necessary.
We should have one source of "truth" as to what URL the client is actually at right now.
Adds local routing, as described in the plan