Add st.navigation, st.Page and associated V2 strategies #8673

kmcgrady · 2024-05-15T04:54:48Z

Describe your changes

This change includes st.Page and st.navigation commands bringing us closer to the finish point for MPA v2.

Testing Plan

Added associated unit tests
E2E Tests will be needed to test MPA end-to-end, but they will not be included in this PR to the feature branch.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

raethlein · 2024-05-15T08:50:15Z

frontend/app/src/App.tsx

+    // If the app logo is not associated with the main script, clear it.
+    if (appLogo && appLogo.activeScriptHash !== mainScriptHash) {


is this something we could already handle on the backend? Like, not allowing to send the logo from anywhere else but the main script? I don't have a super strong opinion here, but it feels a little bit weird on first glimpse to have the page_script stuff being tightly bundled with these different areas, such as logo, on the web app side and think that it might be cleaner to not send it in the first place. It might also help to detach these concepts from each other from the perspective of using the web app more like a lib - the less complex logic, the better 🙂

Ah. I think it should be in the frontend. It's essentially the same logic as the clearPageNodes, but the logo is in a special case.

How about logo be in the AppNode as a special variable? That would definitely clean it up.

frontend/app/src/util/AppNavigation.ts

frontend/lib/src/AppNode.ts

raethlein · 2024-05-15T09:19:31Z

frontend/app/src/util/AppNavigation.ts

+    )
+  }
+
+  clearPageElements(


a more telling name might be retainMainScriptElements or filterMainScriptElements if I understood the logic correctly (and based on this comment)

lib/streamlit/commands/navigation.py

raethlein · 2024-05-15T09:30:29Z

lib/streamlit/commands/navigation.py

+
+@gather_metrics("navigation")
+def navigation(
+    pages: list[StreamlitPage] | dict[str, list[StreamlitPage]],


nit: should the key-type be SectionHeader instead of str?

I would be fine, but do we need to worry for the docs that come from the types. I'll follow up.

lib/streamlit/commands/navigation.py

lib/streamlit/navigation/page.py

raethlein · 2024-05-15T09:49:18Z

lib/streamlit/navigation/page.py

+        if isinstance(self._page, Path):
+            h = calc_md5(str(self._page))
+        else:
+            h = calc_md5(self.title)


if we calc the hash based on the title, we should perhaps add a check in navigation for title uniqueness and raise an error if multiple pages have the same title (maybe I have overlooked it though or this scenario is not possible)

That's a good point. It's hard cause the path and the title can be different, but also provide the same thing. We would need to detect uniqueness of the hashes. I'll talk to @sfc-gh-jcarroll

Addressed this.

vdonato · 2024-05-15T22:12:17Z

lib/streamlit/commands/navigation.py

+    if len(page_list) == 0:
+        raise StreamlitAPIException(
+            "`st.navigation` must be called with at least one `st.Page`."
+        )


nit: the idiomatic way of writing this is just to write if page_list since the empty list is Falsy

vdonato · 2024-05-15T22:17:40Z

lib/streamlit/commands/navigation.py

+            "`st.navigation` must be called with at least one `st.Page`."
+        )
+
+    defaults = []


nit: This feels like a weird way to write this. I feel like it'd be cleaner to initialize default = None, then set it if we encounter a page with the default attribute set to True or error out if default is already set

vdonato · 2024-05-15T22:29:04Z

lib/streamlit/commands/navigation.py

+    managed_page = ctx.pages_manager.get_page_script(
+        fallback_page_hash=default_page._script_hash
+    )
+
+    found_page = default_page


managed_page and found_page seem like weird names to me / I feel like had a hard time figuring out what's going on here due to them

Would something like:

managed_page -> found_page

found_page -> page_to_return

work?

I also feel like it'd be cleaner to write something like

found_page = ctx.pages_manager.get_page_script( fallback_page_hash=default_page._script_hash ) page_to_return = None if found_page: found_page_script_hash = found_page["page_script_hash"] matching_pages = [ p for p in page_list if p._script_hash == found_page_script_hash ] if len(matching_pages) > 0: page_to_return = matching_pages[0] if not page_to_return: send_page_not_found(ctx) page_to_return = default_page # ...

Also related is my comment above about whether the return value of ctx.pages_manager.get_page_script should ever be falsy at this point.

vdonato · 2024-05-15T22:34:22Z

lib/streamlit/commands/navigation.py

+    # Inform our page manager about the set of pages we have
+    ctx.pages_manager.set_pages(pagehash_to_pageinfo)
+    managed_page = ctx.pages_manager.get_page_script(
+        fallback_page_hash=default_page._script_hash


(Writing this without double-checking the implementation of get_page_script because I think the comment is valid regardless of exactly what the method's exact behavior is)

Is it possible for ctx.pages_manager.get_page_script to return None? If so, it feels weird to me that we're passing it fallback_page_hash=default_page._script_hash, which I would guess should always allow it to return a value since we're both giving it a fallback and are requesting a page that we know at this point must exist.

Yea, perhaps better naming is needed here, but it falls under the weirdness we used for the v1, and is just as complex as v2. We basically search for pages based on:

page_script_hash - if it exists check using this, and otherwise, rely on the default hash, so it should technically always return a page. The edge case is when st.navigation is called with different set of pages. We set the page_script_hash as the page in the first call, but the subsequent calls is with a new set of hashes. We just rely on the new default page identified.

We check by page name - this can return None - This is the direct navigation setup where we rely on the url path to identify the page and will present a Page Not Found

If neither is specified, we get the fallback, it's essentially the default page.

Definitely can write more comments around the call. Perhaps detect_page_script or find_page_script.

vdonato · 2024-05-15T22:41:46Z

lib/streamlit/navigation/page.py

+        if isinstance(page, Path):
+            page = (main_path / page).resolve()


Does this line handle the case where the path is an absolute one? Just looking at this, I would guess that the path constructed here will be incorrect when given an absolute path. If this is the case, we should probably reword the docstring to say the path must be relative rather than it can be.

Ah. Yes. The spec says it should be relative. I'll note that in the docstring.

vdonato · 2024-05-15T23:18:32Z

lib/tests/streamlit/runtime/pages_manager_test.py

+
+    def test_run_with_active_hash(self):
+        """Ensure the active script is set correctly"""
+        current_hash = self.pages_manager.main_script_hash


nit: I feel like main_script_hash would be a better name here since the hash isn't always current throughout the test (it's not in the run_with_active_hash contextmanager, for example)

vdonato · 2024-05-15T23:19:47Z

frontend/app/src/util/AppNavigation.ts

-export class V1Strategy {
+function getTitle(pageName: string): string {
+  return `${pageName} · Streamlit`
+}


nit: newline between function end and new class declaration

vdonato · 2024-05-15T23:20:26Z

frontend/app/src/util/AppNavigation.ts

@@ -38,7 +42,10 @@ export type PageUrlUpdateCallback = (
 ) => void
 export type PageNotFoundCallback = (pageName?: string) => void

-export class V1Strategy {
+function getTitle(pageName: string): string {
+  return `${pageName} · Streamlit`


nit: should we just have this return Streamlit when pageName is falsy? The separator is a bit weird otherwise

vdonato · 2024-05-15T23:22:04Z

frontend/app/src/util/AppNavigation.ts

+  }
+}
+
+export class StrategyV2 {


Should a common interface be defined here so that we can ensure StrategyV1 and StrategyV2 both comply to it? It seems like it'd be easy in the future to have them desync in ways that slip by code review + CI without one.

I was thinking that, but the types were StrategyV1 | StrategyV2, so theoretically this must comply with the interface for both, but let me know if you think strongly.

vdonato · 2024-05-15T23:28:57Z

frontend/app/src/util/AppNavigation.ts

+    this.hideSidebarNav = position === "hidden"
+
+    const currentPage = appPages.find(
+      p => p.pageScriptHash === navigationMsg.pageScriptHash


nit: should we extract pageScriptHash from navigationMsg in the destructor already used above to be used here instead of accessing it from the object?

vdonato · 2024-05-16T07:25:39Z

lib/streamlit/runtime/pages_manager.py

+        try:
+            yield
+        except Exception:
+            self.set_active_script_hash(original_page_hash)
+            raise
+        else:
+            self.set_active_script_hash(original_page_hash)


Worth double checking, but I think writing

try: yield finally: self.set_active_script_hash(original_page_hash)

should be equivalent (I think finally can sometimes essentially catch an exception, but only if a control flow statement is used within the finally block)

## Describe your changes This change includes `st.Page` and `st.navigation` commands bringing us closer to the finish point for MPA v2. ## Testing Plan - Added associated unit tests - E2E Tests will be needed to test MPA end-to-end, but they will not be included in this PR to the feature branch. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

kmcgrady added security-assessment-completed change:feature impact:users labels May 15, 2024

Add st.navigation and associated V2 "strategies"

d793539

kmcgrady force-pushed the feature/st-navigation branch from e93d3e2 to d793539 Compare May 15, 2024 05:08

kmcgrady added the do-not-merge label May 15, 2024

Add tests

92e9e10

kmcgrady force-pushed the feature/st-navigation branch from 0c23a48 to 92e9e10 Compare May 15, 2024 06:12

kmcgrady added 2 commits May 14, 2024 23:24

Add onHistoryChange support

9a60cfa

Fix url pathname

e0aeb1c