Skip to content

Improve handling Var's when passing them as data to rx.download(...)#5961

Closed
riebecj wants to merge 4 commits intoreflex-dev:mainfrom
riebecj:main
Closed

Improve handling Var's when passing them as data to rx.download(...)#5961
riebecj wants to merge 4 commits intoreflex-dev:mainfrom
riebecj:main

Conversation

@riebecj
Copy link
Copy Markdown
Contributor

@riebecj riebecj commented Nov 7, 2025

Related to #5954 and #5957.

When data is provided as a Var, strings could sometimes have formatting issues and bytes were ArrayVar's that were really not handled at all, as there is no such thing as byte-strings in JavaScript. They were correctly converted to byte arrays, but the resultant downloaded file was just a string representation of the byte array.

  • Var[bytes]: The byte array is coerced into a Uint8Array and converted to a Blob using the specified mime_type. The object URL is then created from the Blob.
  • Var[str]: The string data is left intact to preserve formatting.
  • Var[Any]: Any other supplied state or non-state variable is stringify-ed.
  • Base64 Encoded: To ensure correct data translation, the var data (except for bytes) is base64 encoded.

All of this is done within the variable ecosystem, as there seems to be some aversion to changing any of the Reflex embedded JS (not a gripe, just an observation).

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Nov 7, 2025

CodSpeed Performance Report

Merging #5961 will not alter performance

Comparing riebecj:main (ea9f14b) with main (3c3ddc2)

Summary

✅ 8 untouched

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR enhances the rx.download() function to properly handle different types of Var objects when downloading files. The changes address critical formatting issues that occurred when downloading files with string or byte data from state variables. The solution introduces new JavaScript function variables (BASE64_ENCODE and CREATE_OBJECT_URL) and adds a to_blob() method to ArrayVar for converting byte arrays to JavaScript Blob objects.

The implementation distinguishes between three data types: Var[bytes] creates Blob objects using Uint8Array for proper binary downloads, Var[str] preserves string formatting using base64 encoding instead of URL encoding, and Var[Any] stringifies other data types. This approach works within Reflex's existing variable ecosystem rather than modifying embedded JavaScript files, maintaining architectural consistency while solving fundamental data handling problems in file downloads.

Important Files Changed

Filename Score Overview
reflex/vars/sequence.py 3/5 Adds to_blob() method to ArrayVar class for converting arrays to JavaScript Blob objects
reflex/event.py 4/5 Enhances rx.download() function with improved Var handling logic for different data types
reflex/vars/function.py 4/5 Introduces BASE64_ENCODE and CREATE_OBJECT_URL JavaScript function variables

Confidence score: 3/5

  • This PR addresses a legitimate bug but introduces potential security vulnerabilities that require careful review
  • Score reflects concerns about JavaScript injection risks in the to_blob() method where mime_type parameter is directly interpolated into JavaScript expressions without validation
  • Pay close attention to reflex/vars/sequence.py for the security vulnerability in the to_blob() method's f-string interpolation

Sequence Diagram

sequenceDiagram
    participant User
    participant Frontend
    participant Server
    participant VarOperations
    participant FileSystem

    User->>Frontend: "Call rx.download with data parameter"
    Frontend->>VarOperations: "Check data type (bytes/string/Var)"
    
    alt data is bytes
        VarOperations->>VarOperations: "Base64 encode bytes"
        VarOperations->>VarOperations: "Create data URI with mime_type"
    else data is string
        VarOperations->>VarOperations: "Base64 encode UTF-8 string"
        VarOperations->>VarOperations: "Create data URI with mime_type"
    else data is Var[bytes] (ArrayVar)
        VarOperations->>VarOperations: "Convert to Uint8Array"
        VarOperations->>VarOperations: "Create Blob with mime_type"
        VarOperations->>VarOperations: "Generate object URL from Blob"
    else data is Var[str] (StringVar)
        VarOperations->>VarOperations: "Base64 encode string data"
        VarOperations->>VarOperations: "Create data URI"
    else data is Var[Any]
        VarOperations->>VarOperations: "Convert to string representation"
        VarOperations->>VarOperations: "Base64 encode"
        VarOperations->>VarOperations: "Create data URI"
    end
    
    VarOperations->>Server: "Create EventSpec with processed URL"
    Server->>Frontend: "Return download event with URL and filename"
    Frontend->>FileSystem: "Trigger browser download"
    FileSystem-->>User: "File downloaded with correct format"
Loading

3 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment thread reflex/vars/function.py Outdated
Comment thread reflex/vars/function.py Outdated
@riebecj
Copy link
Copy Markdown
Contributor Author

riebecj commented Nov 7, 2025

I don't think the failed test has anything to do with my code changes, but I can update if necessary.

Copy link
Copy Markdown
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the direction here and tightening up the types, but i think we need to make Blob, object url, and base64 encoding more first class.

I'd like to see a BlobVar implementation, that can be created from the to_blob method on appropriate var types. And then create_object_url could be a method on the BlobVar to get a StringVar with the temporary object path.

I think it would also be cool if the StringVar had a to_base64 and from_base64 methods that properly handled unicode strings (the current BASE64_ENCODE helper would blow up on unicode characters.

With these in place, the behavior of rx.download would be: pass a StringVar, either we use it directly as a data: or blob: url OR we base64 encode it as a data uri with the given mime_type. if you pass a BlobVar, then we give you an object url to download, and any other kind of var would result in JSON (how it currently is handled today). If users need other type conversions, those should be implemented as Var operation methods the user is expected to translate with before calling rx.download.

At the end of the day, we need very consistent and understandable behavior with this API. It should strive to not make implicit conversions; so I think the behavior mentioned above would give everyone what they need.

Comment thread reflex/event.py Outdated
@riebecj
Copy link
Copy Markdown
Contributor Author

riebecj commented Nov 7, 2025

@masenf, I can work on that.

What about handling of Python bytes? Currently, nothing in the Var system is designed to ensure handling of Python byte strings to JS byte arrays. Since bytes are a Python primitive, I would suggest that it gets first class treatment before Blobs, urls, and base64 encoding. If the idea is to NOT handle bytes and require conversion to str, it should not be an accepted type anywhere.

@riebecj riebecj marked this pull request as draft November 10, 2025 14:38
@riebecj
Copy link
Copy Markdown
Contributor Author

riebecj commented Nov 10, 2025

Converting to draft to use as a work space for BytesVar and BlobVar.

This will be abandoned and a new one created when the architecture and implementation of the referenced vars is agreed upon.

@riebecj
Copy link
Copy Markdown
Contributor Author

riebecj commented Nov 10, 2025

@masenf , Take a look at BytesVar and BlobVar. Let me know where things should be changed/improved/etc.

I ran it through multiple example tests using direct State vars, computed vars, and even creating them directly. All seem to function correctly, download with proper formatting, and supports Unicode.

@riebecj
Copy link
Copy Markdown
Contributor Author

riebecj commented Nov 10, 2025

All the type hinting seems to be there too.

I was a little wary about using encode and decode because of the underlying _decode for Vars, but it felt the most Pythonic given its implementation type.

Copy link
Copy Markdown
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, this looks quite good. i left a couple of comments.

hopefully this work makes it easier to deal with blobby things, like media streams, in reflex

Comment thread reflex/vars/blob.py Outdated
Comment thread reflex/vars/blob.py
@riebecj
Copy link
Copy Markdown
Contributor Author

riebecj commented Nov 11, 2025

@masenf Made the changes you suggested.

@riebecj
Copy link
Copy Markdown
Contributor Author

riebecj commented Nov 12, 2025

@masenf Any more suggestions?

@riebecj riebecj closed this Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants