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

Sidebars #18

Closed
Ashoat opened this issue May 29, 2020 · 16 comments
Closed

Sidebars #18

Ashoat opened this issue May 29, 2020 · 16 comments
Assignees

Comments

@Ashoat
Copy link
Contributor

Ashoat commented May 29, 2020

Sidebars are a kind of child thread. They're meant for short back-and-forth conversations that start in a longer thread, but don't need to include everybody on that parent thread. In that sense they're somewhat similar to Slack threads.

You can create one by pressing on any message in a chat thread. People in the parent thread won't get notifs about messages in the child thread unless they press through to the child thread and choose to join it.

One crucial difference between the existing kind of child threads and sidebars is how they appear in the thread list. Whereas child threads are their own top-level entity, sidebars appear grouped below their parent thread. You only see the sidebars in which you are a member. If a new message occurs in a sidebar that you are a member, it bumps the whole parent thread to the top of the thread list.

For this reason, whereas standard child threads can have their own child threads, sidebars can not.

If the discussion in a sidebar drags on, and it starts to look like a long-term conversation, it's possible for any user to "upgrade" the sidebar to a standard child thread. If a sidebar gets upgraded, it will retain its link to the message in the parent thread that started it, but will otherwise behave like a standard child thread, appearing as its own entity in the thread list.

@Ashoat Ashoat mentioned this issue May 29, 2020
@Ashoat
Copy link
Contributor Author

Ashoat commented Jul 28, 2020

Steps:

  1. Introduce new thread type
  2. Server logic for creating/updating sidebars
    • Role types
    • Make sure to disable threadPermissions.EDIT_ENTRIES
    • Updating should change permissions
  3. native MessageList
    • Logic for creating new sidebars (implemented in D579)
    • Way of showing sidebars inline next to their originating message (in progress D653)
    • Updates to RobotextMessage to mention "sidebar" instead of "child thread"
  4. native ChatThreadList
    • Display sidebars inline
    • Limit how many sidebars are displayed on-screen
    • Thread children modal
    • Implement Swipeable for sidebars
  5. native ThreadSettings
    • Option to promote to subthread
    • Distinct icon
    • Way to see all of a thread's sidebars, separate from the list of child threads
  6. web ChatMessageList
    • Logic for creating new sidebars
    • Way of showing sidebars inline next to their originating message
    • Updates to RobotextMessage to mention "sidebar" instead of "child thread"
  7. web ChatThreadList
    • Display sidebars inline
    • Limit how many sidebars are displayed on-screen
    • Thread children modal
  8. Add new permission type for creating sidebars, separate from the one for creating subthreads
    • Use new permission when creating sidebars (on client and server)
    • Check subthreads permission when promoting (on client and server)

@Ashoat Ashoat self-assigned this Jul 28, 2020
@Ashoat
Copy link
Contributor Author

Ashoat commented Dec 1, 2020

Some additional work:

  1. Let's only allow adding users that are in the parent thread
  2. Let's stop showing sidebars inline if they're not unread and haven't had any activity in a while (maybe 3 days?)
  3. That implies that we might have sidebars, but not be showing any inline. In that case we'll still show the "See more" button to pull up SidebarListModal, but let's title it "See sidebars"
  4. Let's find a way to show the message that started the sidebar at the top of the sidebar's MessageList New message type SIDEBAR_SOURCE was introduced for this purpose

The first three should be easy. The fourth might be harder. I'm okay shipping sidebars before we finish the fourth, and then following up on it afterwards.

@Ashoat
Copy link
Contributor Author

Ashoat commented Dec 21, 2020

We should also make sure to think about how thread search works with sidebars

@Ashoat
Copy link
Contributor Author

Ashoat commented Dec 25, 2020

I looked into how thread search deals with sidebars right now:

  • It looks like threadSearchIndex includes sidebars in its results, but because reduxChatListData doesn't include sidebars at the top level, sidebars are never included in the search results
  • If a thread matches, its sidebars are show in the results

I think we'll eventually need to rework the search results view, but I opened a new issue for that: #122. For now, let's make sure that we:

  1. Don't show sidebars below matching top-level threads in the search results
  2. Show matching sidebar results as if they were top-level threads

@Ashoat
Copy link
Contributor Author

Ashoat commented Jan 14, 2021

Some more stuff:

  1. Notif text: "X started a sidebar in response to Y's message 'what should we do?'" (making sure to truncate the message text) We decided to display sidebar name instead of the message. This name should be set to message content.
  2. Robotext: "X started this sidebar [and added Y, Z]" (don't include the author of the initial message as added)
  3. Thread title: the same as the message text, but make sure to truncate it in the same way we do the notif text. We can either set this on the client side and pass it to newThread on the server in ChatInputBar, or have the server determine it based on the sourceMessageID that it gets passed

@KatPo
Copy link
Contributor

KatPo commented Jan 14, 2021

Things that need to be done:

  • creating sidebars from text message (in progress ready)
  • creating sidebars from multimedia message (in progress ready)
  • creating sidebars from robotext message (requires new tooltip) (in progress ready)
  • permission check for message by blocked viewer - right now pending sidebar is displayed with blocked input (in progress ready)
  • showing sidebars inline next to original message (in progress)

All of this needs to be done for web also

@Ashoat
Copy link
Contributor Author

Ashoat commented Jan 14, 2021

One tiny thing that @palys-swm is going to get to later: in D573 he added a new function called newThreadRobotext that handles only CREATE_THREAD and CREATE_SIDEBAR, but typed it using RobotextMessageInfo. I requested that he uses a more precise type, but he preferred to defer this type fix to a separate diff. I'm documenting it here so we can both track this work.

EDIT

This was done today!! Had so many diffs to review that I missed it initially. A great problem to have :)

@Ashoat
Copy link
Contributor Author

Ashoat commented Jan 15, 2021

One thought I had just now: right now we're passing the sourceMessageID to the server, and relying on the server to fetch the message. But in the future when we have end-to-end encryption, the server won't be able to read the contents of the message. So I wonder if we should update our newThread API to actually take the whole sourceRawMessageInfo instead.

@palys-swm
Copy link
Collaborator

Things that need to be done:

  • We changed unsupported sidebar source message to first message in sidebar, but because we prepend creator before every unsupported message, it is displayed like e.g. you first message in sidebar. We need to fix that.
  • We need to set sidebar thread title on the frontend. It should be based on source message and be truncated if needed. Also, all quotation marks should be discarded.

@Ashoat
Copy link
Contributor Author

Ashoat commented Jan 19, 2021

I realized another thing we'll need to change this morning. We want to display number of replies in the sidebar inline under the message that started the sidebar in the parent thread. But currently, only members of the sidebar have the information to determine the number of messages in that sidebar. I think the best solution is probably to make sure messages get delivered to members of the parent that aren’t members of the sidebar.

@KatPo
Copy link
Contributor

KatPo commented Jan 20, 2021

  • Promoting sidebar to sub-thread message displays "you updated the thread's type to 3", we need to be more specific and say something like "you promoted sidebar to subthread"
  • Creating sidebar from pending, promoting it to subthread immediately and going back results in pending thread being displayed fixed in creating sidebars diff

@Ashoat
Copy link
Contributor Author

Ashoat commented Jan 21, 2021

Some more stuff:

  1. Regarding counting the number of messages in the sidebar for the purposes of displaying the count in the parent thread: I realized I need to think about this more.
    • One issue is that we still want to show the "N replies" link in the parent for a sidebar that has been promoted to a sidebar, so our rule of "also deliver sidebar messages to members of the parent" won't be enough... we would need to change it to "also deliver the first N messages in threads with sourceMessageID to members of the parent", which is a slightly different.
    • An alternative would be to make an invisible message type for the parent that wouldn't show up in the UI, but would just update the count of messages in the sidebar. This avoids sending unnecessary data, but maybe is a bit more complicated than it needs to be.
    • Finally, there's the consideration of how we handle this once we have E2E encryption. In that world, clients won't be able to dynamically fetch messages from the server for a thread that they weren't in when the message was created. If we want members of the parent to be able to see all messages in a sidebar, that's a strong argument for sending sidebar messages to the parent members. But it opens this bigger question of how to handle promoted sidebars. It seems weird if sidebars get messages delivered and then those messages stop being delivered once the sidebar is promoted...
    • I need to spend some more time thinking about this...
  2. @KatPo pointed out that right now we're only checking for sidebars in ConnectedMessageListContainer in D579, but if we want to guarantee only one thread per sourceMessageID (including sidebars that have been promoted to subthreads), we should be checking for all threads there. -> done, now it's not possible to have more then one thread with given sourceMessageID
  3. Small thing worth mentioning: once we are displaying the "N replies" link inline in the MessageList underneath a message that started a sidebar, we might want to reconsider the "Create sidebar" button in the tooltip. We can either disable the button (since the sidebar is already created), or maybe better would be to rename it to "Go to sidebar". Renaming should probably be enough... my understanding from reading the code is that clicking that button when a sidebar already exists should just lead to the existing sidebar.
  4. If somebody reads a message they are interested in hearing more about, but doesn't want to reply yet, there should be a way for them to "subscribe" to that message so that when a sidebar is created, they automatically join. Once we have the E2E layer/keyservers set up, this is something we can implement on the user's keyserver... basically it will watch that sourceMessageID and if/when a ThreadInfo is created for it, it will automatically join. For now we can maybe hold off on implementing this.

@Ashoat
Copy link
Contributor Author

Ashoat commented Jan 25, 2021

We'll also need to think about redux-persist migrations for our new message types to handle the unshimming on the client side, along with making sure we have the right codeVersion for when we should be shimming on the server side.

@Ashoat
Copy link
Contributor Author

Ashoat commented Jan 25, 2021

Two questions popped up last week, one about how we reference the sourceMessage in an E2E-encrypted world, and the other about how to measure the number of replies in the sidebar.

I'm going to summarize where we landed on this stuff here. Some explanation of terminology before I get into it:

  1. A "thin thread" is a thread that is hosted explicitly on one keyserver, and the other users' keyservers don't need to track it. Any given user can always fetch missing content by asking the hosting keyserver for it. This is how threads work in SquadCal today, only instead of a hosting keyserver we just have our single server instance.
  2. A "thick thread" is a thread that isn't hosted anywhere specifically, and ownership is split across all participants. This is similar to how threads work in Signal or WhatsApp today. When we initially launch these, there won't be an easy way for a user to fetch missing threads. Most existing threads in SquadCal today will need to be migrated to being "thick threads" once we launch E2E encryption.

Now let me get into the two questions from last week:

  1. For sourceMessage we are going to keep the API where the client passes just the ID to the server.
    1. For thick threads, the user's keyserver will be keeping track of every message they receive in that thread and in any sidebars.
      • Instead of the server fetching and delivering the SIDEBAR_SOURCE message, and including the full RawMessageInfo in the SidebarSourceMessageData and RawSidebarSourceMessageInfo, it will just include the ID.
      • The user's keyserver will match up that message ID with what it sees locally. If the user's keyserver does not have that message ID locally, that indicates that the sidebar was created from a message that was sent before the user joined the thread, and consequently we won't show this sidebar to the user.
      • Let's update the RawMessageInfo and MessageData SIDEBAR_SOURCE to take just message ID.
      • Let's also update the client so that the MessageInfo for SIDEBAR_SOURCE includes the full sourceMessage from the MessageStore, even though the RawMessageInfo will only have the ID.
      • We'll also need to update the MessageStorePruner so that it doesn't prune any messages for thick threads.
    2. For thin threads, we have no guarantee that the user's keyserver has the message in question. In these scenarios, the server will need to send down the source message along with the SIDEBAR_SOURCE.
      • We can include the RawMessageInfo for the source message along with the others we send to the client in this scenario.
      • We'll need to update MessageStorePruner on the client side so that it doesn't prune messages where there is another message that isn't pruned that refers to it. This will make sure we don't prune the messages that the SIDEBAR_SOURCE needs in order to render until the SIDEBAR_SOURCE itself is pruned. We'll probably need some reverse index to make this work.
      • message-reducer.js will also need to be updated, since right now I think it cleans out RawMessageInfos that don't have a reference from messageIDs.
  2. Regarding keeping track of the number of messages in a sidebar:
    1. For thick threads, we will make sure that every message in a sidebar is delivered to the users that are in the parent thread.
      • We had concerns about how to handle sidebars that have been promoted to a subthread. To address this I think the easiest solution will just be to disable subthreads for thick threads for now. We can have subthreads be a feature that is limited to thin threads, eg. communities that are hosted on a particular user's keyserver.
      • In the case of users that were not in the parent thread when the sidebar was created, since they won't be able to see the sourceMessage anyways, we can conclude that they won't be able to see the sidebar, and won't have any permissions for the sidebar.
    2. For thin threads, we'll need to do something different.
      • For thin threads, somebody might not be getting the messages for the sidebar until they join it.
      • Maybe we can create a new message type for the parent thread that will just contain an update for the number of messages in the sidebar. We can deliver this message type to members of the parent thread that are not members of the sidebar.

@Ashoat
Copy link
Contributor Author

Ashoat commented Jan 25, 2021

Separate note:

At this point this GitHub issue is getting very long and it's hard to track the remaining work. Unfortunately I simply don't have time to handle the tracking work. @palys-swm – I'm hoping you can take over for me. As a starting point it would be helpful if you could break down the remaining work we have left to get sidebars shipped.

@palys-swm
Copy link
Collaborator

palys-swm commented Jan 26, 2021

The list with all the things that are required for the release:
Native & server:

  1. Display sidebars inline next to their originating message (in progress D653)
  2. Make sure that all the rebotexts mentioning sidebar use sidebar instead of child thread Fixed in create-sub-thread-message-spec and create-sidebar-message-spec
  3. Set thread title to the source message content (Karol can do that as his first task) (with quotation marks removed and truncated if necessary)(text message handled in D666, other types still to do)
  4. When creating a real thread from a pending, send a name if it was set for the pending thread
  5. Fix a bug which causes unsupported message to display incorrectly you first message in sidebar(D665)
  6. Display a number of messages inline under the source message
  7. Display correct message when promoting sidebar to a subthread (including name instead of number)(D660)
  8. Replace create sidebar with go to sidebar in the tooltip when sidebar exists (and navigate to sidebar instead of creating the new one) (D663)
  9. Write redux-persist migration for SIDEBAR_SOURCE and CREATE_SIDEBAR (D664)
  10. Fix a bug where logging out with opened sidebar causes undefined is not an object exception (D670)
  11. Add option to join a sidebar After clicking on a sidebar, there's a join thread button

Web:
Basically implement the whole sidebar solution. That includes, but is not limited to:

  1. Logic for creating new sidebars
  2. Display sidebars inline next to their originating message
  3. Make sure that all the rebotexts mentioning sidebar use sidebar instead of child thread
  4. Make sure that sidebar name is set appropriately

Things that can be implemented later:

  1. Improve searching (handled in Improve search results view #122)
  2. Add an option to subscribe to a message: this would result in automatically joining the sidebar when it's created (we can create a new task for it)
  3. Enhanced sourceMessage and number of messages handling Sidebars #18 (comment). Probably part of it will be done as a part of 6. Display a number of messages inline under the source message

@benschac benschac closed this as completed Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants