Details

Reviewers

atul
ginsu
ashoat

Commits

rCOMM992a50e084d1: [lib] Update types for SIDEBAR_SOURCE messages to reflect isInvalidSidebarSource

Summary

Now that isInvalidSidebarSource is currently the single source of truth for the valid/invalid message types that can be sidebar sources, it makes sense to update the types for
SidebarSourceMessageInfo SidebarSourceMessageData and RawSidebarSourceMessageInfo to reflect this.

Resolves https://linear.app/comm/issue/ENG-5207/update-types-for-sidebar-source-messages-to-reflect

Depends on D9322

Test Plan

Ran flow to confirm that updating the sourceMessage types didn't cause issues, and manually tested sidebar creation again for test / change role messages / toggle pin messages.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Sep 28 2023, 12:47 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptSep 28 2023, 12:47 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 28 2023, 12:49 PM

Harbormaster failed remote builds in B22905: Diff 31536!

I forgot to include the changes for ValidSidebarSourceMessageInfo in the initial commit

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 28 2023, 1:01 PM

Harbormaster failed remote builds in B22906: Diff 31537!

I'll rerun CI when it's back up, requesting review in the meantime

rohan added a child revision: D9325: [lib] Introduce a canBeSidebarSource function in each message spec.Sep 28 2023, 3:47 PM

Harbormaster completed remote builds in B22906: Diff 31537.Sep 28 2023, 5:41 PM

Harbormaster completed remote builds in B22905: Diff 31536.Sep 28 2023, 5:53 PM

seems reasonable

This revision is now accepted and ready to land.Sep 29 2023, 12:43 PM

ashoat mentioned this in D9325: [lib] Introduce a canBeSidebarSource function in each message spec.Oct 3 2023, 9:12 AM

rohan added a parent revision: D9322: [keyserver/lib] Dedup isMessageSidebarSourceReactionEditOrPin into isInvalidSidebarSource.Oct 10 2023, 7:58 AM

rohan edited the summary of this revision. (Show Details)

rohan removed a parent revision: D9323: [keyserver/lib] Move isInvalidSidebarSource from message-utils to message-types.

Rebase

Harbormaster completed remote builds in B23146: Diff 31873.Oct 10 2023, 8:21 AM

rohan edited the summary of this revision. (Show Details)Oct 10 2023, 8:59 AM

ashoat added inline comments.Oct 14 2023, 11:52 AM

lib/shared/messages/sidebar-source-message-spec.js
90–93 ↗	(On Diff #31873)	This `invariant` feels dangerous.,. you're going to cause the keyserver to crash if a source message is not present in the database. It looks like the previous behavior was to print a warning in that case. Can you check whether the keyserver already crashes in that scenario? If it doesn't already crash, AND you feel confident that `isInvalidSidebarSource` by itself should never trigger, THEN I wonder if we can change this to: invariant( !sourceMessage \|\| isInvalidSidebarSource(sourceMessage), 'SIDEBAR_SOURCE should not point to an invalid message type', );

rohan added inline comments.Oct 17 2023, 5:01 PM

lib/shared/messages/sidebar-source-message-spec.js
90–93 ↗	(On Diff #31873)	Played around with `rawMessageInfoFromServerDBRow`, and it looks like we don't crash if `sourceMessage` is void/null. I've not experienced the `invariant` causing a crash yet, but I'd like to ideally avoid it in the worst case. I'll probably need to figure out a better way to appease Flow then

Make return type from rawMessageInfoFromServerDBRow optional. Inspired by how we handle this in createSubThreadMessageSpec. Here, we won't cause the keyserver to crash through an invariant, and the callsites for rawMessageInfoFromServerDBRow already expect the return type to be ?RawMessageInfo

Re-requesting review because I've changed the logic here

Harbormaster completed remote builds in B23318: Diff 32127.Oct 17 2023, 5:27 PM

CREATE_SUB_THREAD does work that way, but it's very special-cased...

One thing that's worth understanding is how unread status works in the MariaDB database. Basically we have a last_message and a last_read_message, and if they're equal the chat is deemed to be "read".

There's a risk here if we're not delivering to the user the message marked as last_message, then they won't set that message ID for last_read_message.

CREATE_SUB_THREAD is special-cased in message-creator.js not to bump last_message for users that don't have permission to see the created channel.

Note that it's also special-cased in the message-fetchers.js SQL queries, but I don't think that applies here.

Anyways, I think a better option is to keep the old behavior. If we can't get a valid source message, still return a message, but give it an undefined sourceMessage. Can you confirm that in that case, the client will still get that message, and bump last_read_message? (Or whatever that SQL column is named)

This revision now requires changes to proceed.Oct 18 2023, 4:53 PM

Address feedback. Tested against three different scenarios:

Original behavior (i.e. what master looks like for rawMessageInfoFromServerDBRow)
Artifically forcing sourceMessage to be undefined
New behavior (the code in this diff)

In all three cases, I compared the values for last_message and last_read_message in the memberships table for a user, and observed that the values always got correctly updated in all three scenarios, both for normal text messages and when a sidebar is created. Both the rows for the sidebar in memberships and for the parent thread in memberships were always correct

rohan added inline comments.Oct 19 2023, 11:31 AM

lib/shared/messages/sidebar-source-message-spec.js

87 ↗

(On Diff #32223)

I should be able to make this null if reviewer's feel strongly if I change

export type SidebarSourceMessageData = {
  +type: 17,
  +threadID: string,
  +creatorID: string,
  +time: number,
  +sourceMessage?: ValidRawSidebarSourceMessageInfo,
};

export type SidebarSourceMessageData = {
  +type: 17,
  +threadID: string,
  +creatorID: string,
  +time: number,
  +sourceMessage?: ?ValidRawSidebarSourceMessageInfo,
};

Harbormaster completed remote builds in B23376: Diff 32223.Oct 19 2023, 11:44 AM

ashoat accepted this revision.Oct 19 2023, 12:03 PM

This revision is now accepted and ready to land.Oct 19 2023, 12:03 PM

Closed by commit rCOMM992a50e084d1: [lib] Update types for SIDEBAR_SOURCE messages to reflect isInvalidSidebarSource (authored by rohan). · Explain WhyOct 20 2023, 8:27 AM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM992a50e084d1: [lib] Update types for SIDEBAR_SOURCE messages to reflect isInvalidSidebarSource.