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

message_edit: Fix local echo of message edit. #29338

Merged
merged 1 commit into from
May 2, 2024

Conversation

roanster007
Copy link
Collaborator

@roanster007 roanster007 commented Mar 18, 2024

Previously, when a message is edited, it is locally echoed with its
pre-edit content.

This is because previously, when we tried to render the edited
message of the edit box during local echo, in order to update
the content, flags, and is_me_message properties of the message
object with that of those returned is markdown.render(), we used
the spread operator and created a new message object, and updated
the existing message object with this new one.

This was misconverted, since edit_locally() method already has a
fully-rendered message object to start with, and is just doing a
rerendering, it should be mutating what message it received, rather
than constructing a new local variable.

CZO

Screenshots and screen captures:

brave_54ij6LYfVT

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@roanster007 roanster007 marked this pull request as draft March 18, 2024 20:48
@roanster007 roanster007 force-pushed the iss-local-echo branch 2 times, most recently from bfe3793 to a171007 Compare March 19, 2024 08:10
@roanster007 roanster007 marked this pull request as ready for review March 19, 2024 08:11
message_containers = messages.map((message) => {
const message_container = this.message_containers.get(message.id);
message_container.msg.content = message.content;
return message_container;
Copy link
Sponsor Member

@timabbott timabbott Mar 19, 2024

Choose a reason for hiding this comment

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

I think we need to be careful with mutating the core message object here; this may persist more than we'd expect. I'm not sure what the right strategy is, but I think it's pretty likely that if the edit operation fails, this doesn't get undone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it won't be much of a problem. If the patch request for save_message_row_edit fails, we can see that the original content is being restored, and the message_container is being re-rendered here, with original content and message_content_edited being set to false.

Manually testing by stopping the server and editing, it didn't seem like a problem either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sure this isn't the right fix for this. I guess I'll give it another try to fix it.

@alya
Copy link
Contributor

alya commented Mar 20, 2024

@timabbott Marking for integration review since you seem to be looking at this one, and there's a response to your comment above.

@alya alya added the integration review Added by maintainers when a PR may be ready for integration. label Mar 20, 2024
@roanster007 roanster007 force-pushed the iss-local-echo branch 3 times, most recently from 97ded2b to 6801aad Compare March 20, 2024 19:39
Previously, when a message is edited, it is locally echoed with its
pre-edit content.

This is because previously, when we tried to render the edited
message of the edit box during local echo, in order to update
the content, flags, and is_me_message properties of the message
object with that of those returned is markdown.render(), we used
the spread operator and created a new message object, and updated
the existing message object with this new one.

This was misconverted, since edit_locally() method already has a
fully-rendered message object to start with, and is just doing a
rerendering, it should be mutating what message it received, rather
than constructing a new local variable.
@roanster007
Copy link
Collaborator Author

@timabbott this bug still persists. approach is described here

@timabbott timabbott merged commit f999de2 into zulip:main May 2, 2024
7 checks passed
@timabbott
Copy link
Sponsor Member

Great, merged, thanks @roanster007! Probably copying the most important part of the chat.zulip.org thread would have helped me review this more quickly; it took me a while to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when a PR may be ready for integration. size: S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants