Details
- Reviewers
atul marcin - Commits
- rCOMM486d17f33f20: [native] Upload encrypted media to Blob service
Set the useBlobServiceUploads to true. Send a message with image, video, multiple media (mixed images and videos). Verify they're sent successfully. Verify Blob service content (S3 + DynamoDB) and keyserver uploads table. Logged out and in to reset local cache. Media are still loaded correctly.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Looks good, couple of notes inline that would be good to get a response to but not necessarily actionable
| native/input/input-state-container.react.js | ||
|---|---|---|
| 848–856 ↗ | (On Diff #25815) | Wonder if this can be defined outside of call to reduce indentation/improve readability? |
| 865–866 ↗ | (On Diff #25815) | The fact that we're storing thumbnails as JPEGs w/ jpg filename extension seems like an implementation detail, so I'm weary of "hard coding" the file extension and mimeType here. On the other hand, it's unlikely that we'll change for thumbnail format any time soon (maybe webp idk)? And if we were to, it would probably be a "larger" refactor and the developer would probably search for jpg/jpeg/etc? CC @ashoat, but I think this is probably fine for now? |
| native/input/input-state-container.react.js | ||
|---|---|---|
| 865–866 ↗ | (On Diff #25815) | I don't feel strongly, and I'm not familiar with the current code. It looks like this is how it's currently done anyways? (See lines 911/912) |
| native/input/input-state-container.react.js | ||
|---|---|---|
| 848–856 ↗ | (On Diff #25815) | Tried this already, couldn't get Flow working. The problem is that argument types for each call are different and Flow isn't able to see that this is intersecting part for both types |
| 865–866 ↗ | (On Diff #25815) | I have a few ideas for minor improvements here, but they're out of scope now. And yes, this is a copy-paste from L911-12 |