Page MenuHomePhabricator
Differential D6688

[keyserver] Refactoring `redisPublish` function to `messageBrokerPublish`
AbandonedPublic
Actions

Authored by tomek on Feb 10 2023, 7:31 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 9, 6:59 AM
Unknown Object (File)
Tue, Apr 8, 8:42 PM
Unknown Object (File)
Mon, Mar 24, 1:07 PM
Unknown Object (File)
Fri, Mar 21, 8:49 AM
Unknown Object (File)
Mon, Mar 17, 11:39 PM
Unknown Object (File)
Mon, Mar 17, 11:39 PM
Unknown Object (File)
Mon, Mar 17, 11:39 PM
Unknown Object (File)
Mon, Mar 17, 11:39 PM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
atul
• max
Summary

This diff introduces changes to the Keyserver codebase to refactor the redisPublish function name to the messageBrokerPublish to reflect its abstraction type. According to our task, this function will publish not only to the Redis and to the Tunnelbroker.

Linear task: ENG-2613

Test Plan

Passing the CI is enough for these changes.

Diff Detail

Repository
rCOMM Comm
Branch
ks-refactor-publish
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Feb 10 2023, 7:31 AM
• max held this revision as a draft.
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptFeb 10 2023, 7:31 AM
• max added a parent revision: D6460: [keyserver] Adding Tunnelbroker-client napi-rs publish class method.Feb 10 2023, 7:32 AM
Harbormaster completed remote builds in B16327: Diff 22329.Feb 10 2023, 7:47 AM
• max added a child revision: D6689: [keyserver] Refactoring rust-node-addon API names.Feb 10 2023, 8:02 AM
• max retitled this revision from [keyserver] Refactor `redisPublish` function to `messageBrokerPublish` to [keyserver] Refactoring `redisPublish` function to `messageBrokerPublish`.Feb 10 2023, 8:11 AM
• max edited the summary of this revision. (Show Details)Feb 13 2023, 4:47 AM
• max edited the test plan for this revision. (Show Details)
• max added reviewers: atul, tomek.
• max published this revision for review.Feb 13 2023, 5:57 AM
atul accepted this revision.Feb 13 2023, 9:20 AM

Seems like a straightforward rename, and looks like all usages were fixed:

7e0692.png (666×1 px, 115 KB)

keyserver/src/creators/update-creator.js
381 ↗(On Diff #22329)

Should we rename this to promises.messageBroker?

This revision is now accepted and ready to land.Feb 13 2023, 9:20 AM
• max removed a parent revision: D6460: [keyserver] Adding Tunnelbroker-client napi-rs publish class method.Feb 15 2023, 3:16 PM
• max updated this revision to Diff 22720.Feb 20 2023, 5:22 AM
• max edited the test plan for this revision. (Show Details)

Renaming promises.redis to promises.messageBroker.

• max marked an inline comment as done.Feb 20 2023, 5:23 AM
• max added inline comments.
keyserver/src/creators/update-creator.js
381 ↗(On Diff #22329)

Should we rename this to promises.messageBroker?

Totally makes sense! I've renamed it. Thanks, @atul!

Harbormaster failed remote builds in B16614: Diff 22720!Feb 20 2023, 5:26 AM
• max updated this revision to Diff 22723.Feb 20 2023, 5:40 AM
• max marked an inline comment as done.

Fixing parent stack.

Harbormaster failed remote builds in B16617: Diff 22723!Feb 20 2023, 8:38 AM
• max updated this revision to Diff 22994.Feb 23 2023, 5:43 AM

Rebasing on master and parent changes.

Harbormaster completed remote builds in B16820: Diff 22994.Feb 23 2023, 5:58 AM
• max removed a child revision: D6689: [keyserver] Refactoring rust-node-addon API names.Feb 23 2023, 6:51 AM
tomek commandeered this revision.Apr 9 2024, 10:57 AM
tomek abandoned this revision.
tomek edited reviewers, added: • max; removed: tomek.

Not sure if we still need this - the Linear issue has been canceled for a long time.

Revision Contents
Changeset List

PathSize
keyserver/
src/
creators/
4 lines
7 lines

Diff 22723

View Options

keyserver/src/creators/message-creator.js

  • This file was added.
// @flow
import invariant from 'invariant';
import Alert from 'react-native/Libraries/Alert/Alert';
import {
sendReactionMessage,
sendReactionMessageActionTypes,
} from 'lib/actions/message-actions';
import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';
import type { TooltipRoute } from '../navigation/tooltip.react';
function onPressReact(
route:
| TooltipRoute<'TextMessageTooltipModal'>
| TooltipRoute<'MultimediaMessageTooltipModal'>,
dispatchFunctions: DispatchFunctions,
bindServerCall: BindServerCall,
) {
const messageID = route.params.item.messageInfo.id;
const threadID = route.params.item.threadInfo.id;
tomekUnsubmitted
Not Done
Inline Actions

This approach seems to be overly complicated. Can't we just write a function that receives messageID, threadID and reaction? This function can be named after an action that is performed, so react or sendReaction.

tomek: This approach seems to be overly complicated. Can't we just write a function that receives…
invariant(messageID, 'messageID should be set');
invariant(threadID, 'threadID should be set');
const callSendReactionMessage = bindServerCall(sendReactionMessage);
const reactionMessagePromise = (async () => {
try {
const result = await callSendReactionMessage(threadID, messageID, '👍');
return {
targetMessageID: messageID,
serverID: result.id,
threadID,
time: result.newMessageInfo.time,
interface: result.interface,
newMessageInfos: [result.newMessageInfo],
};
} catch (e) {
Alert.alert(
tomekUnsubmitted
Not Done
Inline Actions

It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.

tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
ginsuAuthorUnsubmitted
Done
Inline Actions

Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.

ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
'Couldn’t send the reaction',
'Please try again later',
[{ text: 'OK' }],
{
cancelable: false,
tomekUnsubmitted
Not Done
Inline Actions

Is there a reason to set cancelable to false?

tomek: Is there a reason to set cancelable to false?
ginsuAuthorUnsubmitted
Done
Inline Actions

Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context

I also found the alert on report-utils to follow the same patern

ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
},
);
throw e;
}
})();
dispatchFunctions.dispatchActionPromise(
sendReactionMessageActionTypes,
reactionMessagePromise,
);
}
export { onPressReact };
View Options

keyserver/src/creators/update-creator.js

  • This file was added.
// @flow
import invariant from 'invariant';
import Alert from 'react-native/Libraries/Alert/Alert';
import {
sendReactionMessage,
sendReactionMessageActionTypes,
} from 'lib/actions/message-actions';
import type { BindServerCall, DispatchFunctions } from 'lib/utils/action-utils';
import type { TooltipRoute } from '../navigation/tooltip.react';
function onPressReact(
route:
| TooltipRoute<'TextMessageTooltipModal'>
| TooltipRoute<'MultimediaMessageTooltipModal'>,
dispatchFunctions: DispatchFunctions,
bindServerCall: BindServerCall,
) {
const messageID = route.params.item.messageInfo.id;
const threadID = route.params.item.threadInfo.id;
tomekUnsubmitted
Not Done
Inline Actions

This approach seems to be overly complicated. Can't we just write a function that receives messageID, threadID and reaction? This function can be named after an action that is performed, so react or sendReaction.

tomek: This approach seems to be overly complicated. Can't we just write a function that receives…
invariant(messageID, 'messageID should be set');
invariant(threadID, 'threadID should be set');
const callSendReactionMessage = bindServerCall(sendReactionMessage);
const reactionMessagePromise = (async () => {
try {
const result = await callSendReactionMessage(threadID, messageID, '👍');
return {
targetMessageID: messageID,
serverID: result.id,
threadID,
time: result.newMessageInfo.time,
interface: result.interface,
newMessageInfos: [result.newMessageInfo],
};
} catch (e) {
Alert.alert(
tomekUnsubmitted
Not Done
Inline Actions

It sounds risky to show one alert after every failed message. A user might react to a lot of messages in short period and a network issue could result in a lot of alerts being shown. Other apps sometimes choose to silently fail when a reaction fails.

tomek: It sounds risky to show one alert after every failed message. A user might react to a lot of…
ginsuAuthorUnsubmitted
Done
Inline Actions

Totally see your concern here! I spoke to @atul about this today, and we both agreed that we want this to be a temporary solution, but since reaction messages don't have retry behavior we think it is best to show some sort of feedback to the user in case something failed.

ginsu: Totally see your concern here! I spoke to @atul about this today, and we both agreed that we…
'Couldn’t send the reaction',
'Please try again later',
[{ text: 'OK' }],
{
cancelable: false,
tomekUnsubmitted
Not Done
Inline Actions

Is there a reason to set cancelable to false?

tomek: Is there a reason to set cancelable to false?
ginsuAuthorUnsubmitted
Done
Inline Actions

Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by default and can be enabled by providing an optional Options parameter with the cancelable property set to true. Here is more context

I also found the alert on report-utils to follow the same patern

ginsu: Alerts on Android can be dismissed by tapping outside of the alert box. It is disabled by…
},
);
throw e;
}
})();
dispatchFunctions.dispatchActionPromise(
sendReactionMessageActionTypes,
reactionMessagePromise,
);
}
export { onPressReact };