feat: improve quote event loading [SQSERVICES-1834] #14632

tlebon · 2023-02-06T11:33:36Z

SQSERVICES-1834 "You cannot see this message." in reply to own message

PR Submission Checklist for internal contributors

The PR Title
- conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
- contains a reference JIRA issue number like SQPIT-764
- answers the question: If merged, this PR will: ... ³
The PR Description
- is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Tries a different approach to solve the 'message not found' bug in #14630. That solution caused some unexpected side effects.

Causes (Optional)

As loadEvent is more generic than expected, it is causing some issues in various locations.

Solutions

The solution to this is to create other methods to be consumed in specific locations.

This reverts commit 7b775ef.

codecov · 2023-02-08T14:53:24Z

Codecov Report

Merging #14632 (b41c83a) into dev (5a87c27) will increase coverage by 0.16%.
The diff coverage is 30.43%.

@@            Coverage Diff             @@
##              dev   #14632      +/-   ##
==========================================
+ Coverage   42.60%   42.76%   +0.16%     
==========================================
  Files         615      618       +3     
  Lines       21041    21132      +91     
  Branches     4806     4839      +33     
==========================================
+ Hits         8965     9038      +73     
+ Misses      10960    10959       -1     
- Partials     1116     1135      +19

przemvs · 2023-02-09T08:22:25Z

src/script/entity/Conversation.ts

+    return this.messages().find(messageEntity => (messageEntity as ContentMessage)?.replacing_message_id === messageId);
+  }


Maybe use isContentMessage instead of as ? 🤔

przemvs

Code LGTM, 1 comment ;)

atomrc · 2023-02-09T08:33:29Z

src/script/components/MessagesList/Message/MessageWrapper.tsx

@@ -78,7 +78,7 @@ export const MessageWrapper: React.FC<MessageParams & {hasMarker: boolean; isMes
  isMsgElementsFocusable,
 }) => {
  const findMessage = (conversation: Conversation, messageId: string) => {
-    return messageRepository.getMessageInConversationById(conversation, messageId, true, true);
+    return messageRepository.getMessageInConversationByReplacementId(conversation, messageId);


Humm I think we also need to get the message by regular ID at this point. The replacing_id should only be a fallback in case we don't find the message by ID

i think the naming is confusing you here but it does indeed to either/or, not only replacingId

atomrc · 2023-02-09T08:34:31Z

src/script/conversation/MessageRepository.ts

   * @returns Resolves with the message
   */
-  async getMessageInConversationById(
+  async getMessageInConversationById(conversation: Conversation, messageId: string): Promise<StoredContentMessage> {
+    const messageEntity = conversation.getMessage(messageId);


I believe the default value of this method is not to look up the local state. This might be a regression here (since now it always lookup to local state)

the default value is false and this is negated. so the default case is always to look up.

atomrc · 2023-02-09T08:36:25Z

src/script/conversation/MessageRepository.ts

@@ -1197,7 +1218,7 @@ export class MessageRepository {
      );
    }

-    if (ensureUser && message.from && !message.user().id) {
+    if (message.from && !message.user().id) {


Let's extract this logic to a different method and have a single responsibility for the getMessageInConversationByReplacementId.
This way the consumer can do:

const event = getMessageInConverstation(..) || getMessageInConversationByReplacementId() const event = loadUsers(event);

i did something but i think based on your comment you would like something different

atomrc · 2023-02-09T08:37:21Z

src/script/event/EventService.ts

      }

      const records = (await this.storageService.getAll(StorageSchemata.OBJECT_STORE.EVENTS)) as EventRecord[];
      return records
-        .filter(record => record.id === eventId && record.conversation === conversationId)
+        .filter(record => record.conversation === conversationId && record.data?.replacing_message_id === eventId)


Let's only lookup by replacing id at this point. The consumer is responsible for falling back to something else if it's not found

in the other function on L140, we are also checking by conversation && then id. (This was also the previous implementation) Should we change that as well or leave both the same?

I assume there was some logic to checking both, although the chances of an overlap here seem relatively astronomical to me. Still sort of unclear when this section of the function would be called anyway.

src/script/event/preprocessor/QuotedMessageMiddleware.ts

atomrc

I think there are some parts that might have introduced behavior changes.

Co-authored-by: Thomas Belin <thomasbelin4@gmail.com>

…e-webapp into fix/revert-message-not-found

atomrc

Seems solid 😎

Revert "runfix: revert 74bdedb (#14629)"

cb39275

This reverts commit 7b775ef.

tlebon requested review from a team and otto-the-bot as code owners February 6, 2023 11:33

otto-the-bot added type: bug / fix 🐞 👕 size: M labels Feb 6, 2023

feat: rearranging things

57632f0

tlebon added 2 commits February 8, 2023 18:28

feat: issue with quotes not found

ead0453

feat: split getMessageInConversationById to handle multiple cases

540cd05

tlebon changed the title ~~runfix: revert message not found reversion~~ feat: improve quote event loading Feb 9, 2023

tlebon changed the title ~~feat: improve quote event loading~~ feat: improve quote event loading [SQSERVICES-1834] Feb 9, 2023

przemvs reviewed Feb 9, 2023

View reviewed changes

przemvs approved these changes Feb 9, 2023

View reviewed changes

chore: switch to using typeguard instead of as

d46d6b4

otto-the-bot added type: feature / request ✨ and removed type: bug / fix 🐞 labels Feb 9, 2023

atomrc reviewed Feb 9, 2023

View reviewed changes

src/script/event/preprocessor/QuotedMessageMiddleware.ts Outdated Show resolved Hide resolved

atomrc reviewed Feb 9, 2023

View reviewed changes

src/script/event/preprocessor/QuotedMessageMiddleware.ts Show resolved Hide resolved

atomrc suggested changes Feb 9, 2023

View reviewed changes

tlebon and others added 5 commits February 9, 2023 15:39

Update src/script/event/preprocessor/QuotedMessageMiddleware.ts

26de135

Co-authored-by: Thomas Belin <thomasbelin4@gmail.com>

chore: resolving one PR comment

4ee3397

Merge branch 'fix/revert-message-not-found' of github.com:wireapp/wir…

a47f44b

…e-webapp into fix/revert-message-not-found

chore: some changes to function structure

d4130bb

test: missing type was breaking things

b41c83a

atomrc approved these changes Feb 16, 2023

View reviewed changes

tlebon merged commit 2ac4ebd into dev Feb 16, 2023

tlebon deleted the fix/revert-message-not-found branch February 16, 2023 13:04

atomrc mentioned this pull request Mar 10, 2023

fix: Improve message sending performances #14800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve quote event loading [SQSERVICES-1834] #14632

feat: improve quote event loading [SQSERVICES-1834] #14632

tlebon commented Feb 6, 2023 •

edited by github-actions bot

Loading

codecov bot commented Feb 8, 2023 •

edited

Loading

przemvs Feb 9, 2023 •

edited

Loading

przemvs left a comment

atomrc Feb 9, 2023

tlebon Feb 9, 2023

atomrc Feb 9, 2023

tlebon Feb 9, 2023 •

edited

Loading

atomrc Feb 9, 2023

tlebon Feb 9, 2023

atomrc Feb 9, 2023

tlebon Feb 9, 2023

atomrc left a comment

atomrc left a comment

		return this.messages().find(messageEntity => (messageEntity as ContentMessage)?.replacing_message_id === messageId);
		}

feat: improve quote event loading [SQSERVICES-1834] #14632

feat: improve quote event loading [SQSERVICES-1834] #14632

Conversation

tlebon commented Feb 6, 2023 • edited by github-actions bot Loading

PR Submission Checklist for internal contributors

What's new in this PR?

Issues

Causes (Optional)

Solutions

codecov bot commented Feb 8, 2023 • edited Loading

Codecov Report

przemvs Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

przemvs left a comment

Choose a reason for hiding this comment

atomrc Feb 9, 2023

Choose a reason for hiding this comment

tlebon Feb 9, 2023

Choose a reason for hiding this comment

atomrc Feb 9, 2023

Choose a reason for hiding this comment

tlebon Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

atomrc Feb 9, 2023

Choose a reason for hiding this comment

tlebon Feb 9, 2023

Choose a reason for hiding this comment

atomrc Feb 9, 2023

Choose a reason for hiding this comment

tlebon Feb 9, 2023

Choose a reason for hiding this comment

atomrc left a comment

Choose a reason for hiding this comment

atomrc left a comment

Choose a reason for hiding this comment

tlebon commented Feb 6, 2023 •

edited by github-actions bot

Loading

codecov bot commented Feb 8, 2023 •

edited

Loading

przemvs Feb 9, 2023 •

edited

Loading

tlebon Feb 9, 2023 •

edited

Loading