We can move this invariant earlier in the function since height will be the height of the inline engagement for multimedia messages

I explained why I added the View here:

https://phab.comm.dev/D8610#inline-55215

When there is no inline engagement, inlineEngagementHeight will just be 0

When there is no inline engagement, inlineEngagementHeight will just be 0. We could also make this optional if people prefer, but I thought it made more sense just to make this field required

Here is my explanation for why I added inlineEngagementHeight to ChatMultimediaMessageInfoItem:

Multimedia messages needed to be handled a little bit differently since we use the image height and the composed message width to determine the contentHeight and we actually don't perform a measurement on multimedia messages. (ChatMultimediaMessageInfoItem also uses MultimediaContentSizes and the other message types do not https://github.com/CommE2E/comm/blob/master/native/types/chat-types.js#L51C1-L52C1)

This means that for multimedia messages we only need to measure the inline engagement and then in multimediaMessageItemHeight we can add the contentHeight w/ the height of the inline engagement we get from the NodeHeightMeasurer.

Hope this helps, but please let me know if I need to elaborate more, or consider a different solution for multimedia messages.

This makes sense, I will then rename this function and move it into native/chat/inline-engagement.react.js

but I'm wondering why e.g. isPinned is optional in that case

@rohan do you have a quick answer for this by any chance?

If I remember right, it's because we don't allow robotext messages to be pinned.

So in chat-selectors.js, we only add the isPinned property for the composable message types (text and multimedia).

Because of this, I'd run into flow issues by expecting isPinned to exist all the time for all messages, specifically in the chat-item-height-measurer.react.js file if I removed the optional parameter.

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ chat/chat-item-height-measurer.react.js:178:29

Cannot create NodeHeightMeasurer element because boolean [1] is incompatible with void (due to access of non-existent
property isPinned) [2] in property isPinned of array element of the first parameter of property allHeightsMeasured.
[incompatible-type]

     chat/chat-item-height-measurer.react.js
 [2] 137│           isPinned: item.isPinned,
        :
     175│         itemToMeasureKey={heightMeasurerKey}
     176│         itemToDummy={heightMeasurerDummy}
     177│         mergeItemWithHeight={heightMeasurerMergeItem}
     178│         allHeightsMeasured={measurement.onMessagesMeasured}
     179│         inputState={inputState}
     180│         composedMessageMaxWidth={composedMessageMaxWidth}
     181│         initialMeasuredHeights={measurement.initialMeasuredHeights}

     types/chat-types.js
 [1]  42│   +isPinned: boolean,

The workaround to this was to include isPinned for robotext messages as well, but that didn't seem to make sense since it's not possible. I believe hasBeenEdited does the same thing as I remember checking for why that was optional.

Let me know if that doesn't clear things up / I can answer any more questions!

Ah that makes sense, thank you for your detailed explanation!

Here is my explanation for why I added inlineEngagementHeight to ChatMultimediaMessageInfoItem:

Multimedia messages needed to be handled a little bit differently since we use the image height and the composed message width to determine the contentHeight and we actually don't perform a measurement on multimedia messages. (ChatMultimediaMessageInfoItem also uses MultimediaContentSizes and the other message types do not https://github.com/CommE2E/comm/blob/master/native/types/chat-types.js#L51C1-L52C1)

This means that for multimedia messages we only need to measure the inline engagement and then in multimediaMessageItemHeight we can add the contentHeight w/ the height of the inline engagement we get from the NodeHeightMeasurer.

Hope this helps, but please let me know if I need to elaborate more, or consider a different solution for multimedia messages.

I explained why I added the View here:

https://phab.comm.dev/D8610#inline-55215