godslayerakp/http: Add block to get response body as data:URL (#2426)#17
godslayerakp/http: Add block to get response body as data:URL (#2426)#17supervoidcoder merged 1 commit intoOmniBlocks:masterfrom
Conversation
implements #2364. --------- Co-authored-by: DangoCat[bot] <dangocat@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbitRelease Notes
WalkthroughAdded a new reporter block Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds functionality to retrieve HTTP response bodies as data:URLs, but there are critical logic errors that will prevent the feature from working correctly:
Critical Issues:
- The
blobproperty is never initialized indefaultResponse, causing undefined access - The
resDataUrl()method has Promise caching issues that will return stale data after new requests - Race condition between
clearAll()and blob assignment could cause empty data:URLs - The Promise caching approach is incompatible with the
clearAll()reset pattern
Required Changes:
- Add
blob: nullto thedefaultResponseobject - Fix the Promise caching strategy to work correctly with the request/response lifecycle
- Consider whether data:URL should be pre-computed during
sendRequest()instead of lazy-loaded
Please address these blocking issues before merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| resDataUrl() { | ||
| if (!this.response.dataUrl) { | ||
| this.response.dataUrl = new Promise((resolve) => { | ||
| const blob = this.response.blob; |
There was a problem hiding this comment.
🛑 Logic Error: Undefined property access. The blob property is never initialized in defaultResponse (line 216-226), causing this.response.blob to be undefined when accessed here. This will result in an empty being returned even when a valid response exists.
Add blob: null to the defaultResponse object in the static getter around line 222.
| resDataUrl() { | ||
| if (!this.response.dataUrl) { | ||
| this.response.dataUrl = new Promise((resolve) => { | ||
| const blob = this.response.blob; | ||
| if (blob) { | ||
| const reader = new FileReader(); | ||
| reader.onloadend = () => resolve(reader.result); | ||
| reader.onerror = () => resolve(""); | ||
| reader.readAsDataURL(blob); | ||
| } else { | ||
| resolve(""); | ||
| } | ||
| }); | ||
| } | ||
| return this.response.dataUrl; | ||
| } |
There was a problem hiding this comment.
🛑 Logic Error: The method creates and caches a Promise but lacks async declaration. Scratch reporters expect either synchronous values or promises from async methods. This implementation returns a Promise that resolves to a string, but the caching logic will cause subsequent calls to return the same Promise object even if clearAll() resets the response. This could lead to stale values being returned after new requests.
| this.response.blob = await res.blob(); | ||
| const body = await this.response.blob.text(); |
There was a problem hiding this comment.
🛑 Logic Error: After clearAll() is called at line 670, this.response.blob will be reset but this.response.dataUrl (the cached Promise) won't exist in the new defaultResponse object. This means the Promise caching in resDataUrl() won't work correctly across multiple requests - the first call after a new request will recreate the Promise even though the blob might not be set yet. The blob is set later at line 717, creating a race condition where resDataUrl() might be called before the blob is available.
2 participants
implements TurboWarp#2364.