Allow sending messages in 1:1 conversations. Create a conversation if it doesn't exist.
Details
Details
- Reviewers
kamil ashoat - Commits
- rCOMMb03c9fdd8472: [lib] Create Farcaster 1:1 threads
Used the thread creator, selected a Farcaster user without 1:1 with me. Sent a message - a thread was created. Both thread and message appeared on Farcaster.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Overall looks good, but I have a lot of comments so requesting changes
| lib/hooks/input-state-container-hooks.js | ||
|---|---|---|
| 42 ↗ | (On Diff #50432) | Perhaps we should rename this to rawThreadInfos for clarity |
| 92 ↗ | (On Diff #50432) | This one might be a little ambiguous too |
| 117 ↗ | (On Diff #50432) | Interesting that sendMultimediaMessage takes RawThreadInfo but sendTextMessage takes ThreadInfo |
| lib/shared/threads/protocols/farcaster-thread-protocol.js | ||
| 120 ↗ | (On Diff #50432) | If we only need RawThreadInfo, then I would adjust the types to always pass ThreadInfo. Then we can align the APIs of useInputStateContainerSendTextMessage and useInputStateContainerSendMultimediaMessage – the former no longer needs to take ThreadInfo as an input, since it's fetching all of the RawThreadInfos anyways |
| 834–836 ↗ | (On Diff #50432) | Should this be extracted? I think it's done in one other places in the file |
| 838 ↗ | (On Diff #50432) | You're mixing Fid and FID` styles throughout. I think FID is our preferred convention |
| 841 ↗ | (On Diff #50432) | Do we do this anywhere else? I wonder if it should be extracted |
| lib/utils/create-farcaster-raw-thread-info.js | ||
| 216 ↗ | (On Diff #50432) | Should we use stringForUserExplicit here? |
| 217 ↗ | (On Diff #50432) | Is this what the Farcaster protocol does? Do they support descriptions for 1-on-1 chats? I think we should probably mirror whatever they're doing |
| 226 ↗ | (On Diff #50432) | Why is unread always false here? |
Comment Actions
Address the review
| lib/hooks/input-state-container-hooks.js | ||
|---|---|---|
| 117 ↗ | (On Diff #50432) | Yeah, this is inconsistent, but a lot of places would be affected by a refactoring that would improve it. If we want to use RawThreadInfo then there's a problem with text messages - in keyserver protocol we're using side effect functions, and updating the types there starts a cascade of changes. And some functions can't be updated easily. If we want to use ThreadInfo, then there's a problem with multimedia messages. In the input state container, we're creating ThreadInfo for text messages by hand, but the flow of creating new threads from multimedia messages is a lot more complicated, e.g. due to uploads. I don't think this refactoring is worth destabilizing the app. |
| lib/shared/threads/protocols/farcaster-thread-protocol.js | ||
| 120 ↗ | (On Diff #50432) | I'm a bit confused by this comment.
If we only need RawThreadInfo, then why pass the ThreadInfo? I'm guessing the intention was to use RawThreadInfo instead. This also isn't trivial, because the thread might be pending, and it isn't included in rawThreadInfos in that case. Aligning the input state hooks is complicated - explained in the previous comment. I've already spent more than an hour trying to align the types - if you think it is worth more investment, then please let me know. But I don't think it is a good idea. |
| 834–836 ↗ | (On Diff #50432) | extracted |
| 838 ↗ | (On Diff #50432) | fixed |
| 841 ↗ | (On Diff #50432) | extracted |
| lib/utils/create-farcaster-raw-thread-info.js | ||
| 216 ↗ | (On Diff #50432) | Yes, we can. |
| 217 ↗ | (On Diff #50432) | I don't think they support the description. We can use an empty string instead. |
| 226 ↗ | (On Diff #50432) | It's the current user who creates the thread, so setting it to unread makes sense. Soon after the creation, this thread will be replaced by the downloaded Farcaster conversation, so none of these values matter too much. |