Page MenuHomePhabricator

[lib] Added new 'sendEditMessage' action
ClosedPublic

Authored by kuba on Mar 21 2023, 11:36 AM.
Tags
None
Referenced Files
F3374998: D7129.id24063.diff
Tue, Nov 26, 5:46 PM
F3374965: D7129.id23956.diff
Tue, Nov 26, 5:35 PM
Unknown Object (File)
Fri, Nov 22, 9:37 PM
Unknown Object (File)
Wed, Nov 13, 5:11 PM
Unknown Object (File)
Sun, Nov 10, 9:03 PM
Unknown Object (File)
Sun, Nov 10, 6:26 PM
Unknown Object (File)
Sun, Nov 10, 4:54 PM
Unknown Object (File)
Sun, Nov 10, 4:52 PM
Subscribers

Details

Summary

Added new action to send edit messages.

Test Plan

Checked if the app still works. Other tests are in later diffs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/types/redux-types.js
566–570 ↗(On Diff #23956)

Flow has an issue where it doesn't recognize that this is an invalid type. Here we are saying that the type is BOTH Error and an exact object type having only three keys: threadID, targetMessageID, and text. That should be impossible because the first type implies other keys should exist.

We can maybe address this by making the second type "inexact"

lib/types/redux-types.js
567–569 ↗(On Diff #23956)

Why do we need these? Do we use them in the reducer?

kuba marked an inline comment as done.

Change type to inexact

lib/types/redux-types.js
567–569 ↗(On Diff #23956)

We don't need it now. But in the future, when I add a state for editing mode on native & web, we will probably need that information to inform the user about editing failure.

ashoat requested changes to this revision.Mar 24 2023, 10:57 AM
ashoat added inline comments.
lib/types/redux-types.js
567–569 ↗(On Diff #23956)

We need more detail than this. I don't understand why it's necessary. It's generally possible to inform users about editing failures without need things in Redux. We only do this for other message types when we need messageStore.local to get updated, but it's not clear to me that you need that.

In general, you should understand 100% of the code you are submitting and why. If you aren't sure yet how this is going to be used, then you should not introduce it now. When you have the context to explain how / why it's going to be used, that's when you can introduce it.

This revision now requires changes to proceed.Mar 24 2023, 10:57 AM
kuba marked 2 inline comments as done.

Removed redundant error payload & moved redux types to the end of the file

ashoat added inline comments.
lib/types/message-types.js
575 ↗(On Diff #24274)

Not sure we need this type alias, but I don't feel strongly

This revision is now accepted and ready to land.Mar 28 2023, 6:00 AM