Fix reqwest caching images on the playground tests #1228
Merged
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 8da1d3f in 51 seconds
More details
- Looked at
142lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. engine/baml-runtime/src/lib.rs:231
- Draft comment:
Consider changing the log level to debug or removing this log statement before merging to avoid cluttering production logs. This applies to other similar log statements added in this file. - Reason this comment was not posted:
Confidence changes required:50%
The logging statements added in the BamlRuntime implementation are useful for debugging but should be removed or changed to a lower log level (e.g., debug) before merging to avoid cluttering the logs in production environments.
2. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:64
- Draft comment:
If the wasm_logger initialization breaks logging, consider removing this line entirely to avoid confusion. - Reason this comment was not posted:
Confidence changes required:50%
The commented-out line for initializing the wasm_logger is noted as potentially breaking logging. If this is the case, it should be removed entirely to avoid confusion.
3. engine/baml-schema-wasm/tests/test_file_manager.rs:227
- Draft comment:
Consider removing the commented-out line if it's not needed to keep the test code clean. - Reason this comment was not posted:
Confidence changes required:30%
The test functiontest_run_testsin the wasm test file has a commented-out line for running a test. If this line is not needed, it should be removed to keep the test code clean.
Workflow ID: wflow_yw8TqJvA1lo3htst
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| let request = if let Some(proxy) = proxy_url { | ||
| client.get(proxy).header("baml-original-url", url) | ||
| client | ||
| .get(format!("{}/{}", proxy, url)) |
There was a problem hiding this comment.
The URL construction with the proxy might be incorrect. Typically, a proxy URL is used to route requests, not concatenate with the original URL. This could lead to incorrect URL formation.
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.
reqwest caches image requests since the base_url is always empty. Adding the path of the original url into the proxy url fixes it.