Details
Starting a thread with someone without CSAT by sending multimedia message no longer fails on web and native
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
[please read inline comments first] An alternative approach would be to create a hook from migrateMessageMediaToKeyserver(), pass it as a prop to input-state-containers and call right before this.props.sendMultimediaMessage(). It could be easier to pass upload metadata on web and update IDs in input-state-containers, but this hook would also need to check for thread type etc (part of the stuff that is done by existing useInputStateContainerSendMultimediaMessage().
lib/hooks/input-state-container-hooks.js | ||
---|---|---|
128 ↗ | (On Diff #44638) | Re. Concern 1: Would this be okay? See below |
258 ↗ | (On Diff #44638) | Concern 1: It works without this, but I have a feeling that this value should be further exported to the callsites of useInputStateContainerSendMultimediaMessage(). |
262 ↗ | (On Diff #44638) | I guess this can be done with Promise.all |
279–290 ↗ | (On Diff #44638) | Another "hack" - I'm taking advantage of keyserver not doing anything with the metadata, besides deducing MediaType basing on MIME type. It would be better to somehow pass original upload data:
|
337–347 ↗ | (On Diff #44638) |
|
lib/hooks/input-state-container-hooks.js | ||
---|---|---|
337–347 ↗ | (On Diff #44638) | I see formatting broke here.
|
- We should avoid awaiting in sequence
- Unclear to me why the client is establishing a holder here. Seems like the keyserver should be doing it instead
lib/hooks/input-state-container-hooks.js | ||
---|---|---|
250 ↗ | (On Diff #44638) | If we decide we need this, then I'd like to understand why you chose to make the inner collections mutable. Was this intentional? |
258 ↗ | (On Diff #44638) | Hmm, I have the same suspicion, but I don't know the code well enough to be able to confirm this off the top of my head. Could you do the following:
|
262 ↗ | (On Diff #44638) | Yes, we should basically never await in a loop unless we have a very good reason to (eg. something can't be parallelized, or we want to batch some operation) |
274 ↗ | (On Diff #44638) | Isn't this backwards? |
279–290 ↗ | (On Diff #44638) |
By this, you mean that it would be better to pass the original MIME type instead of a fake one? I'm personally not very concerned about passing a fake MIME type, since you seem to have confirmed that it doesn't really matter... (right?) |
313 ↗ | (On Diff #44638) | This can be combined with the above await, right? |
337–347 ↗ | (On Diff #44638) |
I suppose by this you mean the local ID already exists, and the server ID has not been determined yet since legacySendMultimediaMessage / sendMultimediaMessage haven't been called yet.
You're suggesting one dispatch per media, right? I'm not sure if there would be any benefits to this... dispatch isn't an async call, so it seems like migrateMessageMediaToKeyserver would still take pretty much the same time to complete. |
We discuss it in D13500
lib/hooks/input-state-container-hooks.js | ||
---|---|---|
250 ↗ | (On Diff #44638) | In my initial approach it used to be mutable, but if we decided we want to return this, I'll rather make it immutable, thanks |
279–290 ↗ | (On Diff #44638) | Yes, it doesn't really matter, I wanted to explain why I did this. |
313 ↗ | (On Diff #44638) | Yes |
337–347 ↗ | (On Diff #44638) |
Yes
I meant only code organization change: moving this into the previous loop (to-be-migrated into Promise.all) instead of doing another for loop. |
lib/hooks/input-state-container-hooks.js | ||
---|---|---|
337–347 ↗ | (On Diff #44638) | Oh, I missed that it's already multiple dispatchs. Yeah, I think that's a good idea |
lib/hooks/input-state-container-hooks.js | ||
---|---|---|
258 ↗ | (On Diff #44638) | I analyzed web code and basically, after sendMultimediaMessage is called, it only checks if the ID exists or not, it doesn't read its value anymore if it exists. |
274 ↗ | (On Diff #44638) | After getting confused twice 😅 I think this is correct.
It's a developer fault if we have ,media which is keyserver hosted (non-blob) and doesn't have keyserver ID. Either way, I'll update this message to be less confusing |
- Parallelized promises where possible
- Removing local holders after media processing
- Updated invariant messages
- Updated some types (exported, made readonly etc)
lib/hooks/input-state-container-hooks.js | ||
---|---|---|
316 | I would if the code would be more readable if processMediaResult didn't have to consider this case:
newMedia.push(media); return; I don't feel too strongly, so you could skip this feedback if you need to move fast. | |
385 | Nit: shorthand |