improve Run App url matching and fix preview timeout issue #5349
Conversation
This file contains 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
### Addresses - #5197 - #5306 ### Implementation Notes #### URL Matching - introduces `appUrlStrings` to the run and debug app options, which is an optional array of `AppUrlString`s, which we will attempt to extract the app url from - `AppUrlString` is a string type that has the placeholder template `{{APP_URL}}` in the string, which indicates the location of the app url relative to the string - adds the appropriate `appUrlStrings` for each framework we support - expands on our url matching by: - attempting to match url-like text where `{{APP_URL}}` is found in the provided `appUrlStrings` - if matching against `appUrlStrings` fails or no `appUrlStrings` are found, we fallback to a more basic url match for strings that start with http or https #### Shell Integration Warning Message This PR should also fix a timing issue where the `didPreviewUrlTimeout` would time out before `terminalOutputTimeout`, causing the shell integration warning message to show. We now set `didPreviewUrlTimeout` to be 5 seconds longer than `terminalOutputTimeout`, so that it doesn't timeout before the app preview is done. ### QA Notes This PR fixes Run App in Terminal url detection for non-local URLs. One way to get a non-local url is by running the `shiny-py-example` in Positron on Workbench (see #5197). Other app types like Dash, Streamlit, Fastapi, Flask and Gradio should continue to work on Desktop, Server Web and Positron on Workbench. This PR should also fix the issue seen in #5306, which can be tested by running the [Dash Py Example](https://github.com/posit-dev/qa-example-content/tree/main/workspaces/dash-py-example) several times to check that the issue does not occur.
The version of prettier we're currently using in the Python extension doesn't recognition string template types. Upgrading prettier results in a lot of formatting changes across many files, so I'm reverting back to a string literal array.
Prettier error:
```
Checking formatting...
src/client/positron-run-app.d.ts[error] src/client/positron-run-app.d.ts: SyntaxError: Type expected. (35:28)
[error] 33 | * more on literal type inference.
[error] 34 | */
[error] > 35 | export type AppUrlString = `${string}{{APP_URL}}${string}`;
```
|
I'm realizing that the Python CI tasks don't run in the other branches, so I missed the prettier formatting checks in the patch branch PR. I opened #5352 to add the prerelease branches to the PR CI workflow. The version of prettier used in the Python extension doesn't support string template types :( Instead of upgrading prettier, which results in a bunch of formatting fixes across multiple files, I've removed the string template type. I'll need to backport any fixes here to the patch branch. Full test suite run: https://github.com/posit-dev/positron/actions/runs/11821745434
|
seeM
approved these changes
Nov 13, 2024
sharon-wang
changed the title
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Cherry-picks #5336 to bring the fix from the 2024.11 patch branch to the
mainbranch. The below description is copied from #5336 and modified to remove the AppUrlString type changes.The prerelease branch will need to be updated again to include the additional changes.
Addresses
Implementation Notes
URL Matching
appUrlStringsto the run and debug app options, which we will attempt to extract the app url from{{APP_URL}}, which indicates the location of the app url relative to the stringappUrlStringsfor each framework we support{{APP_URL}}is found in the providedappUrlStringsappUrlStringsfails or noappUrlStringsare found, we fallback to a more basic url match for strings that start with http or httpsShell Integration Warning Message
This PR should also fix a timing issue where the
didPreviewUrlTimeoutwould time out beforeterminalOutputTimeout, causing the shell integration warning message to show. We now setdidPreviewUrlTimeoutto be 5 seconds longer thanterminalOutputTimeout, so that it doesn't timeout before the app preview is done.QA Notes
This PR fixes Run App in Terminal url detection for non-local URLs. One way to get a non-local url is by running the
shiny-py-examplein Positron on Workbench (see #5197).Other app types like Dash, Streamlit, Fastapi, Flask and Gradio should continue to work on Desktop, Server Web and Positron on Workbench.
This PR should also fix the issue seen in #5306, which can be tested by running the Dash Py Example several times to check that the issue does not occur.