-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Strip mentions from forwarded messages #30884
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
Strip mentions from forwarded messages #30884
Conversation
twassman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suboptimal — should probably actually recalculate the m.mentions based on message body (without reply relation), but not entirely clear to me what 'correct' design would be
As there is no EditorModel, attachMentions() currently does nothing
twassman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This runs forwarded messages through attachMentions() — which just effectively sets m.mentions to empty, as there is no EditorModel for the forwarded message. This feels like the least-bad way to fix #30883 in alignment with the existing codebase, while leaving room for future improvements. Actually determining the m.mentions based on the message body would likely require some degree of structural changes.
dbkr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you're working on tests - thanks. This could probably do with a test itself.
add empty mentions to expected content
|
Linter and tests are fine on my end; not sure why they're failing in the checks. |
|
You probably need to pull the latest changes as I think this is now trying to import something tat no longer exists. I pulled latest changes from develop but unfortunately this wasn't sufficient. |
|
Retrying with a fresh install, it seems like the issue may stem from: I am not entirely sure where this dependency is even located, but it does seem to get flagged in the
|
|
@twassman those errors seem related to testcontainers, not any runtime/build dependency for Element Web Your issue is
https://github.com/element-hq/element-web/tree/develop/packages/shared-components & https://github.com/element-hq/element-web/blob/develop/package.json#L86 |
|
Thank you, @t3chguy; that helps. For whatever reason, even just trying to run typecheck on the develop branch itself is still failing, but I've updated the import & hopefully it should work now on your end. |
Fixes #30883
Checklist
public/exportedsymbols have accurate TSDoc documentation.