Skip to content

feat: DH-21898: Local routing#1348

Merged
mofojed merged 34 commits into
deephaven:mainfrom
jnumainville:21898_local_routing_impl
May 20, 2026
Merged

feat: DH-21898: Local routing#1348
mofojed merged 34 commits into
deephaven:mainfrom
jnumainville:21898_local_routing_impl

Conversation

@jnumainville
Copy link
Copy Markdown
Collaborator

Adds local routing, as described in the plan

@jnumainville jnumainville requested review from dsmmcken and mofojed May 8, 2026 18:00
@jnumainville jnumainville self-assigned this May 8, 2026
@github-actions github-actions Bot requested a review from margaretkennedy May 8, 2026 18:00
@jnumainville jnumainville requested a review from Copilot May 11, 2026 13:43
@jnumainville jnumainville changed the title feat:DH-21898: Local routing feat: DH-21898: Local routing May 11, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, plus route/router components and link(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.

Comment thread plugins/ui/docs/hooks/use_navigate.md
Comment thread plugins/ui/docs/components/link.md
Comment thread tests/app.d/ui_routing.py
Comment thread plugins/ui/src/deephaven/ui/hooks/use_navigate.py
Comment thread plugins/ui/docs/hooks/use_params.md
Comment thread plugins/ui/docs/components/router.md
Comment thread plugins/ui/docs/components/router.md
Comment thread plugins/ui/src/deephaven/ui/components/router.py Outdated
Comment thread plugins/ui/src/deephaven/ui/components/link.py
Comment thread plugins/ui/docs/components/router.md Outdated
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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume "greedily" will be a familiar term to your target audience?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally yes, but there's a clearer way to phrase it anyways.

Copy link
Copy Markdown
Contributor

@margaretkennedy margaretkennedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs look good, minor comments

Comment thread plugins/ui/docs/components/router.md Outdated
Comment thread plugins/ui/docs/components/router.md Outdated
@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@github-actions
Copy link
Copy Markdown

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"])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main reason was the incremental change from query params. I'll rework the logic to be on the python side.

"""
return self._root.get_query_params()

def get_path(self) -> str:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto - we should just be storing this all as the whole URL, and parse from the same source.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked

@@ -35,3 +35,35 @@ def get_query_params(self) -> QueryParams:
def set_query_params(self, query_params: QueryParams) -> None:
"""Update the URL query parameters."""
...
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@jnumainville jnumainville requested a review from mofojed May 19, 2026 13:35
@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

Consider using [`action_button`](./action_button.md) for task-based actions.
```

## Screenshots
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about adding a few style notes? "prefer active voice", "sentence case for headings" if you want the docs to match the other user guides on deephaven.io?

Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also some e2e tests failing it looks like

Args:
url_state: Dict with URL state fields __queryParams,
__path, __absolutePath, __fragment, __href.
url_state: Dict with __url field containing the full URL.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need a __url param at all, we should just pass the URL as the first parameter.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked

Comment on lines +615 to +617
# contexts (which never carry __url) don't accidentally clear URL state.
if "__url" in state:
self._root.set_url(state.pop("__url"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here? I don't think it's necessary, the URL state is already set explicitly with setUrlState?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was vestigial code that is unnecessary after reworks, where the state was passed through in a different way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't catch this in the URL params review, but instead of polluting the component state dictionary with a __url param, instead pass the component state as the first param, and then the URL as the second param. We shouldn't need to use a private key like __url.
If we really wanted to leave the door open for possibly more stuff getting passed, we could say the first param is component state, second parameter is app state, which could just be a typed dict with url as a key (no need for __url or polluting the component state). Then we can add more to the second parameter without polluting the component state at all.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked, good catch.

@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@github-actions
Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@jnumainville jnumainville requested a review from mofojed May 20, 2026 20:52
@mofojed mofojed merged commit a42efa4 into deephaven:main May 20, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants