Make document basepath configurable via window variable#11537
Merged
Make document basepath configurable via window variable#11537
Conversation
vdonato
added
security-assessment-completed
impact:internal
Contributor
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
Contributor
✅ PR preview is ready!
|
vdonato
force-pushed
the
vdonato/configurable-window-basepath
branch
from
c519a7e to
726f150
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
vdonato
force-pushed
the
vdonato/configurable-window-basepath
branch
from
726f150 to
38dd9d8
Compare
vdonato
force-pushed
the
vdonato/configurable-window-basepath
branch
from
38dd9d8 to
5e7fdcb
Compare
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Introduces a new DOCUMENT_BASE_URL window variable and refactors URI parsing to handle multipage apps when the document base path differs from the asset host.
- Add
DOCUMENT_BASE_URLto the global Streamlit window object. - Rename and centralize URI parsing logic (
getBaseUriParts→parseUriIntoBaseParts). - Update
Appand connection utilities/tests to respectDOCUMENT_BASE_URLfor navigation.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/utils/src/types/index.ts | Add DOCUMENT_BASE_URL field to StreamlitWindowObject. |
| frontend/connection/src/utils.ts | Rename getBaseUriParts to parseUriIntoBaseParts. |
| frontend/connection/src/utils.test.ts | Update tests to use parseUriIntoBaseParts. |
| frontend/connection/src/index.ts | Export parseUriIntoBaseParts. |
| frontend/connection/src/DoInitPings.tsx | Use parseUriIntoBaseParts for host config. |
| frontend/app/src/App.tsx | Branch on DOCUMENT_BASE_URL for history handling. |
| frontend/app/src/App.test.tsx | Add tests covering DOCUMENT_BASE_URL scenarios. |
Comments suppressed due to low confidence (3)
frontend/utils/src/types/index.ts:44
- [nitpick] Consider adding a brief comment explaining the semantics of
DOCUMENT_BASE_URL, e.g., that it should point to the app's root URL for multipage routing.
DOCUMENT_BASE_URL?: string
frontend/connection/src/utils.ts:25
- [nitpick] Add a JSDoc comment for
parseUriIntoBasePartsdescribing its behavior (especially the trimming of leading/trailing slashes and default port assignment).
export function parseUriIntoBaseParts(url?: string): URL {
frontend/app/src/App.tsx:1082
- [nitpick]
pathnameis reused in multiple scopes — consider renaming to something likedocumentBasePathto clarify its origin when branching onDOCUMENT_BASE_URL.
let pathname
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
In #11378, we left some TODOs to come back and fix page URL handling for multipage
apps in the world where we configure the client to hit a different origin for the Streamlit
server from the one that we fetched our
index.htmlfrom. This PR addresses these issuesby defining a new
MAIN_PAGE_BASE_URLfield in the__streamlitobject which lets usexplicitly specify what the base URL for the document (that is, the URL of the main page of
the app) should be.
Note that this change will also make it possible to fix the annoying issue where initial
healthcheck requests hit a 404 when navigating directly to a subpage of a multipage
app in the pure OSS world, but we don't do so in this PR as it'll also require us to change
how our static asset handler works to no longer serve a truly static
index.htmlbutdynamically set some pieces of it, which would take a nontrivial amount of additional
effort.
Testing Plan
when
window.__streamlit.MAIN_PAGE_BASE_URLis set.Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.