Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://huggingface.co/buckets/gradio/pypi-previews/resolve/13f7a53cb8d18bee36fb57106bc79ce13703be70/gradio-6.10.0-py3-none-any.whlInstall Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@13f7a53cb8d18bee36fb57106bc79ce13703be70#subdirectory=client/python"Install Gradio JS Client from this PR npm install https://gradio-npm-previews.s3.amazonaws.com/13f7a53cb8d18bee36fb57106bc79ce13703be70/gradio-client-2.1.0.tgz |
🦄 change detectedThis Pull Request includes changes to the following packages.
|
|
Looks good but would be great to see if this impacts the js performance metrics at all |
freddyaboulton
added
v: patch
|
I’m not sure if this is the right approach. I really do think it should be loaded up front, lazy loading will just delay the UI and make the user experience worse. It will probably give slightly better load times (when measured) but won’t make the UI fully ready any faster (probably slower because of the waterfall). Additionally this could cause the info not to render in SSR. I just don’t think those dependencies should be in info at all, we should never have added them. If we want to support basic markdown syntax then we can write a tiny utility to render some basic inline stuff. |
Makes sense, but my thinking was removing those markdown dependencies for |
freddyaboulton
left a comment
freddyaboulton
left a comment
There was a problem hiding this comment.
Looks good @dawoodkhan82 ! thanks for addressing the comments. I only have one comment. Would be good to get @pngwn 's review prior to merge.
| info: "Additional info here" | ||
| }); | ||
| const el = result.getByText("Additional info here"); | ||
| const el = await waitFor(() => result.getByText("Additional info here")); |
There was a problem hiding this comment.
Why was this change needed?
There was a problem hiding this comment.
It was needed because when lazy loading. but will remove now!
|
|
||
| // links [text](url) | ||
| result = result.replace( | ||
| /\[([^\]]+)\]\(([^)]+)\)/g, |
There was a problem hiding this comment.
I think we should do this only if the url begins with http, https: or a relative path otherwise you might be able to put arbitrary js in an <a> tag
There was a problem hiding this comment.
Agree in general but i think we should do something like:
check if a protocol exists on the trimmed url ($2) (regex: ^\w+:)
true:- starts with http + https: render the link (
$1+$2) - else: render only the text (
$1)
- starts with http + https: render the link (
false- render the link
This way you can still write google.com, www.google.com but not stuff like:
[xss-js](javascript:alert(document.cookie) )[xss-vb](vbscript:...)[xss-data](data:text/html;base64...)
pngwn
left a comment
pngwn
left a comment
There was a problem hiding this comment.
This looks great @dawoodkhan82, thanks for making those changes!
It is technically breaking but I think it covers the common cases for info and is close enough that we can get away with it. The improvement to all of our components will be huge.
We definitely need to add some unit tests for this one though. Both the individual regular expressions and the render_inlinemarkdown` utility should be tested.
I'd pull the regexes out into exported constants at the top of the file so we can assert on them independently (regex is painful to read, the tests will be great documentation of their function).
We should also make sure we test cases that we don't want to get through like the xss links i posted in the comment below.
4 participants
Description
Improve performance of 14 components that use
Blocktitlebut do not useInfocomponent.Infocomponent importsmarkdown-codewhich imports a bunch of libraries (katex, prism, marked, mermaid). To solve this, this PR lazy loadsInfowhen needed.Closes: #(issue)
AI Disclosure
We encourage the use of AI tooling in creating PRs, but the any non-trivial use of AI needs be disclosed. E.g. if you used Claude to write a first draft, you should mention that. Trivial tab-completion doesn't need to be disclosed. You should self-review all PRs, especially if they were generated with AI.
🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Testing and Formatting Your Code
PRs will only be merged if tests pass on CI. We recommend at least running the backend tests locally, please set up your Gradio environment locally and run the backed tests:
bash scripts/run_backend_tests.shPlease run these bash scripts to automatically format your code:
bash scripts/format_backend.sh, and (if you made any changes to non-Python files)bash scripts/format_frontend.sh