Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial proposal for threads implementation #18169

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions feature-threads.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
## Reflecting challenge conditions
> Giving yourself a timebox of 4-6 hours (at most), investigate how the webapp is currently handling replies and come up with a solution for how to implement threading given the current architecture. Some code examples or other visualization may be helpful to properly explain your thought process.

At first I explored
1. https://github.com/wireapp/wire-webapp/issues
2. https://github.com/wireapp/wire-webapp/pulls

I checked `EventBuilder` https://github.com/wireapp/wire-webapp/blob/dev/src/script/conversation/EventBuilder.ts#L128 - It's quite clear, some common structure and some event type.

I tried to find some features requests related to **threads** or may be channels. I saw that repository exist from at least 2016 and was surprised that there were no requests about **threads**

I quickly figured out that `Conversation` is one of the backbone. It's a `Conversation` when it's 1:1 or in group, it's the same entity, just different `CONVERSATION_TYPE`. **Threads** also will be a new type of `Conversation`. slightly different but the same in common.

To create a **Thread** we minimally need 3 things:
1. **Thread** technically will be a new `Conversation` with a new type.
2. We need a `quoted message` that contains a link to this **Thread**
3. **Thread** will contain a link to the quoted message

At the same moment when we are able to create quoted message we need to have an option to create a linked `Conversation` type **Thread**

## Code point of view

From technical point of view there is a need for at least
1. New menu item **Threads** in a sidebar menu
2. New `CONVERSATION_TYPE`
3. Represent those new type of conversations in `Conversations` list
4. create new action for creating this new type of `Conversation`
5. We need some alteration of a single message so it could be linked to a **Thread**
6. This also will trigger some changes in backend and other files to provide an opportunity for this
7. Update some backend part to be ready to handle those new type of conversations
8. Update single `Conversation` view to be capable of representing new type of conversation
9. Update behaviour of `Message actions`
10. Update localisation

I put `TODO: ` comments in corresponding parts of the app




## Arised qustions

### User experience
1. will we keep current **reply** and add brand new **reply in a thread** or we will only replace old **reply** with a new **reply in thread** ?
2. Will we give an option to a user to change `ConversationDetails` for a `Conversation` with type `thread`, or we will inherit all the settings from a parent `Conversation`?
3. Is it possible to create a thread within another thread (nested thread)?
4. Is it possible to invite users in a thread if they are not invited to a parent `Conversation`
5. Is it possible to change notifications or other setting in a thread only or in a parent only? or changing some settings in a parent will trigger same change in setting of a thread conversation?

I think this questions are debatable and at this moment I don't have a strong opinion on that.
Personally I would prefer keeping existing quotes functionality and add a brand new threads.

### Technical
From https://datatracker.ietf.org/doc/html/rfc9420#name-required-capabilities:

>At a minimum, all members of the group need to support the cipher suite and protocol version in use." - I think this is about backward compatibility between different versions of the product/APIs etc.

From https://datatracker.ietf.org/doc/html/rfc9420#name-reinitialization

>A group may be reinitialized by creating a new group with the same membership and different parameters, and linking it to the old group via a resumption PSK.

As we are introducing a new technical case **linked conversation**, we need to

1. Convey a way of handling `ConversationDetails`
2. Ensure that parent `Conversation` and **Thread** `Conversation` using compatible protocol/version

I started to explore dependencies and was amazed by "Architecture overview" from https://github.com/wireapp/wire-avs but after some exploring I have some open questions

1. If we need to reinitialize conversations - will we reinitialize all parent and child conversations at the same time?
2. Is it possible that parent and thread conversation support different "protocol/version"?

## Conclusion

1. As a complex feature it requires some debates / collaboration
2. Some technical parts are already nailed down such as new `CONVERSATION_TYPE` alterations and some other parts of the code
3. Some user experience questions needs to be decided such as: Can we invite a user into a **Thread** which is not invited in parent conversation? or: Is it possible to have nested thread? Answers to this question will influence feature implementation
4. Some such as compatible protocol/version, reinitialization needs to be communicated with other departments or require more time for investigation


## Other thoughts

1. I explored https://datatracker.ietf.org/doc/html/rfc9420#name-group-creation - I'm very much excited about MLS. Such an amazing structure, I'd happy working on being part of developing following such a high standards.
2. https://wire.com/en/blog/admin-privilege-model-is-broken - When it comes to sensitive data I very much like and support "Zero trust" idea
3. I started to explore dependencies and was amazed by "Architecture overview" from https://github.com/wireapp/wire-avs

## Other ideas notes and improvement suggestion

1. Explored little bit a group creating thinking about channels creation which are also yet not implemented `src/script/components/Modals/GroupCreation/GroupCreationModal.tsx`
2. Add some useful information to the `Conversations` displaying for example some part of the last message
3. Split settings->options into **Appearance**, **Notifications** and **Sounds**
4. Move **Set a profile color** from a **Profile** to newly created **appearance** tab.
5. Give more deep settings in newly created **sounds** tab. Different sounds on different events, or customised sounds for example
6. Setup different notifications on different users/groups/1:1/threads, for example I want to snooze notification from users a,b,c but have notifications from users d,e,f
7. In `Notifications` `src/script/page/RightSidebar/Notifications/Notifications.tsx` setting give user more options for more granular settings
8. Add icons in `Context menu` (`src/script/ui/ContextMenu.tsx`)
9. Sometimes it was a bit not intuitive to figure out without reading articles https://support.wire.com/hc/en-us/articles/360002855557-Add-a-conversation-to-your-favourites-folder - I believe in a good interface everything should be clear without it.
9 changes: 8 additions & 1 deletion src/script/components/Conversation/Conversation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,14 @@ interface ConversationProps {
}

const CONFIG = Config.getConfig();

/*
* Alex Ishenko
* This file is responsible for represenation of all kinds of conversations,
* This file will require 2 different major updates
* TODO: 1. change display of messages with threads so that user can see other users who participating in a thread, number of messages, link to open thread
* TODO: 2. This component will be responsible for displaying thread conversation itself. add relevant to thread functionality such as changing header maybe displaying entire original message, but it's debbatable
* TODO: split this into smaller parts
*/
export const Conversation = ({
teamState,
selfUser,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,16 @@ export interface ContentMessageProps extends Omit<MessageActions, 'onClickResetS
message: ContentMessage;
onClickButton: (message: CompositeMessage, buttonId: string) => void;
onRetry: (message: ContentMessage) => void;
/*
* Alex Ishenko
* This is the quotedMessage functionality,
* TODO: we will need to refactor this adding OR replacing current functionality
* currently message can have a quotedMessage wich is a link to a parent message
* but for threads we will need a link to a Conversation
* may be this will be a replacement for a quotedMessage
* may be this will be a separate functionality see more in feature-threads.md
* this anyway will trigger some changes in other files which are handling adding message
*/
quotedMessage?: ContentMessage;
selfId: QualifiedId;
isMsgElementsFocusable: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ const MessageActionsMenu: FC<MessageActionsMenuProps> = ({
[currentMsgActionName, handleMenuOpen],
);

/*
* Alex Ishenko
* This is the reply functionality,
* TODO: we need "Reply in Thread" button which will create a new converstaion
*/
const handleMessageReply = useCallback(
(event: React.MouseEvent<HTMLButtonElement>) => {
toggleActiveMenu(event);
Expand Down
7 changes: 6 additions & 1 deletion src/script/conversation/EventBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,12 @@ export const EventBuilder = {
type: ClientEvent.CONVERSATION.FILE_TYPE_RESTRICTED,
};
},

/*
* Alex Ishenko
* TODO: create buildThreadCreation
* that will create thread using newly added
* type: ClientEvent.CONVERSATION.THREAD_CREATION, from @wireapp/api-client/lib/conversation/Conversation.d.ts
*/
buildGroupCreation(
conversationEntity: Conversation,
isTemporaryGuest: boolean = false,
Expand Down
15 changes: 15 additions & 0 deletions src/script/entity/Conversation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ enum TIMESTAMP_TYPE {
MUTED = 'mutedTimestamp',
}

/*
* Alex Ishenko
* Wow!
*/
export class Conversation {
private readonly teamState: TeamState;
public readonly archivedState: ko.Observable<boolean>;
Expand Down Expand Up @@ -172,6 +176,11 @@ export class Conversation {
public readonly showNotificationsNothing: ko.PureComputed<boolean>;
public status: ko.Observable<ConversationStatus>;
public teamId: string;
/*
* Alex Ishenko
* TODO: add new CONVERSATION_TYPE called THREAD
* node_modules/@wireapp/api-client/lib/conversation/Conversation.d.ts
*/
public readonly type: ko.Observable<CONVERSATION_TYPE>;
public readonly unreadState: ko.PureComputed<UnreadState>;
public readonly verification_state = ko.observable(ConversationVerificationState.UNVERIFIED);
Expand All @@ -182,6 +191,12 @@ export class Conversation {
public accessModes?: CONVERSATION_ACCESS[];
public accessRole?: CONVERSATION_LEGACY_ACCESS_ROLE | CONVERSATION_ACCESS_ROLE[];
public domain: string;
/*
* Alex Ishenko
* TODO: Add some more properties depends on feature requirements. for example link to message
* May be name and/or display_name are enough to display the proper header for a thread, may be not
* this may cause also adding relevant functionality in other places and affect API
*/

static get TIMESTAMP_TYPE(): typeof TIMESTAMP_TYPE {
return TIMESTAMP_TYPE;
Expand Down
4 changes: 4 additions & 0 deletions src/script/event/Client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ export enum CONVERSATION {
DELETE_EVERYWHERE = 'conversation.delete-everywhere',
FILE_TYPE_RESTRICTED = 'conversation.file-type-restricted',
GROUP_CREATION = 'conversation.group-creation',
/*
* Alex Ishenko
* TODO: add THREAD_CREATION
*/
INCOMING_MESSAGE_TOO_BIG = 'conversation.incoming-message-too-big',
KNOCK = 'conversation.knock',
LEGAL_HOLD_UPDATE = 'conversation.legal-hold-update',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ export const ConversationTabs = ({
Icon: <GroupIcon />,
unreadConversations: groupConversations.filter(filterUnreadAndArchivedConversations).length,
},
/*
* Alex Ishenko
* TODO: Add menu item for threads
*/
{
type: SidebarTabs.DIRECTS,
title: t('conversationLabelDirects'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,11 @@ type ConversationsProps = {
teamRepository: TeamRepository;
userRepository: UserRepository;
};

/*
* Alex Ishenko
* TODO: split this into smaller parts
* TODO: add relevant to threads functionality such as displaying threads
*/
const Conversations: React.FC<ConversationsProps> = ({
integrationRepository,
searchRepository,
Expand Down
5 changes: 4 additions & 1 deletion src/script/util/LocalizerUtil/LocalizerUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ export const setLocale = (newLocale: string): void => {
export const setStrings = (newStrings: typeof strings): void => {
strings = newStrings;
};

/*
* Alex Ishenko
* TODO: add all relevant translations and new strings
*/
export function t(
identifier: StringIdentifer,
substitutions?: Substitutes,
Expand Down
Loading