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
- Branch
- barthap/blob/keyserver
- 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 | Wonder if this can be defined outside of call to reduce indentation/improve readability? | |
| 865–866 | 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 | 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 | 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 | 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 | |
At this point, nearly every workflow requires additional steps. Maybe we can think about a way to make this seem better in the future -- users should know before reading the footnote that Nix isn't going to do everything for them. But, it will do most of the work for them, and anything else will be smaller.
While the user might still know this, the footnote isn't really helping right now. It makes it seem like we told them Nix will do a lot of things, and then later slid in that they will have to do more things.
Currently, the footnote for these sections has a lot of examples for "additional steps." Plus, "additional steps" is broad and could mean any amount of steps, especially to a user who has no familiarity with the dev environment.
So we should think about that, since most of the time footnotes should be used sparingly. If they end up affecting a large portion of a section, we should just make it information the user knows outside of just the footnote. But this can be a separate issue.