Skip to content

Alkwa/dont render html messages #1149

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.

Sign up for GitHub

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

Merged
alkwa-msft merged 8 commits into from
Nov 24, 2021
Merged

Alkwa/dont render html messages #1149

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
85dffc8
avoid rendering meta messages from Teams
alkwa-msft Nov 24, 2021
dd8c85e
Change files
alkwa-msft Nov 24, 2021
bffabd4
using correct condition to avoid unexpected messages from trouter not…
alkwa-msft Nov 24, 2021
59130c7
removing additional console.log
alkwa-msft Nov 24, 2021
3c8c19c
Merge branch 'main' into alkwa/dont-render-html-messages
anjulgarg Nov 24, 2021
90e5653
account for html type from trouter notification based messages
alkwa-msft Nov 24, 2021
0f5a308
Merge remote-tracking branch 'refs/remotes/origin/alkwa/dont-render-h…
alkwa-msft Nov 24, 2021
56033b4
Merge branch 'main' into alkwa/dont-render-html-messages
alkwa-msft Nov 24, 2021
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
7 changes: 7 additions & 0 deletions change/@internal-chat-stateful-client-53862ccc-050c-43c6-89c2-d356941826e9.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "avoid rendering richtext/media messages until we can properly handle it",
"packageName": "@internal/chat-stateful-client",
"email": "79329532+alkwa-msft@users.noreply.github.com",
"dependentChangeType": "patch"
}
7 changes: 7 additions & 0 deletions packages/chat-stateful-client/src/EventSubscriber.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,14 @@ export class EventSubscriber {
};

private onChatMessageReceived = (event: ChatMessageReceivedEvent): void => {
// Today we are avoiding how to render these messages. In the future we can
// remove this condition and handle this message appropriately.
if (event.type !== 'Text' && event.type !== 'RichText/Html') {
Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need a condition for 'html'? https://github.com/Azure/communication-ui-library/pull/946/files

Looking to avoid breaking: #824

Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: convert the type we are checking to lowercase and use the enum ACSKnownMessageType we have for checking against

Copy link
Copy Markdown
Contributor

@PorterNan PorterNan Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, 'html' type is coming from teams sometimes - not sure if that is also the case for notifications

Copy link
Copy Markdown
Contributor

@PorterNan PorterNan Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge inconsistence of type name/lower/upper cases from chatThreadClient.getMessages() and these notifications, which should be resolved by our chat sdk in their further release

Copy link
Copy Markdown
Contributor Author

@alkwa-msft alkwa-msft Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I am going to toLowerCase() and do checks against text richtext/html and html

all other messages are currently dropped.

Copy link
Copy Markdown
Contributor

@PorterNan PorterNan Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there concerns that we have to do it in stateful client? Since it is our component + selector's responsibility to render the message (and they don't support this type well), messages filter should be happening in messageThreadSelector.tsx - On the other hand, stateful client should always provide accurate data directly coming from js sdk (so developers can read messages and render them themselves even without our implementation)

We already have some filterings there in messageThreadSelector.tsx, I wonder why it is not working, I guess there could be some leakings for our conditions there

Copy link
Copy Markdown
Contributor

@PorterNan PorterNan Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sync with Alex offline, the big limitation for now is we froze api changes so we are not able to support more types in convertEventType() function above - for short term, this change makes a lot of change before we release the lock of api changes
In long term, we will need to open a bug and add new api changes and move this logic into selector after first GA release

return;
}

const newMessage = this.convertEventToChatMessage(event);

// Because of bug in chatMessageReceived event, if we already have that particular message in context, we want to
// make sure to not overwrite the sequenceId when calling setChatMessage.
const existingMessage = this.chatContext.getState().threads[event.threadId]?.chatMessages[event.id];
Expand Down
⚡