Skip to content

Add basic m.thread support#1349

Merged
ajbura merged 13 commits intocinnyapp:devfrom
greentore:feat/basic-thread-support
Aug 15, 2024
Merged

Add basic m.thread support#1349
ajbura merged 13 commits intocinnyapp:devfrom
greentore:feat/basic-thread-support

Conversation

@greentore
Copy link
Copy Markdown
Contributor

@greentore greentore commented Jul 23, 2023

Description

Adds basic m.thread support as described in MSC3440.
Support in this case means that replying to a threaded message will place the reply in the thread in clients that support them.

Partially fixes #257

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 23, 2023

Preview: https://1349--pr-cinny.netlify.app
⚠️ Exercise caution. Use test accounts. ⚠️

@ajbura ajbura self-assigned this Jul 25, 2023
Comment thread src/app/organisms/room/RoomInput.tsx Outdated
Comment on lines +289 to +292
if (replyDraft.relatesTo?.rel_type === "m.thread") {
content['m.relates_to'].event_id = replyDraft.relatesTo.event_id;
content['m.relates_to'].rel_type = "m.thread";
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As we do not support thread in cinny we should also add "is_falling_back": true as we are replying to a message which is part of thread compare to sending a message in thread with client that support them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's the other way around. If we sent "is_falling_back": true, the message would be interpreted as posted to the thread directly with m.in_reply_to being set only for fallback purposes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This time I explicitly set is_falling_back to false. I think it's saner considering we're making a normal reply from Cinny's perspective.

Comment thread src/app/state/roomInputDrafts.ts Outdated
Comment thread src/app/organisms/room/RoomInput.tsx Outdated
@filipesmedeiros
Copy link
Copy Markdown
Contributor

Is any help needed here? I can try to contribute if you think I could help :)

@JuliaSoriaSmith
Copy link
Copy Markdown

Can the reviewers with write access please look at this? This has been open for quite some time and threads are a noticeable missing feature

@greentore
Copy link
Copy Markdown
Contributor Author

I added threaded reply indicators inspired by element-x. Should resolve #1871.

Comment thread src/app/components/message/Reply.css.ts Outdated
@williamkray
Copy link
Copy Markdown

williamkray commented Aug 7, 2024

i'm not an approver but i've briefly tested the netlify deployment of this PR and it looks fantastic, i'm very eager to see this merged.

seems like the quoted message doesn't get truncated with elipses (...) properly, and runs off the right hand side of the screen.

image

@williamkray
Copy link
Copy Markdown

same messages in stable cinny:

image

@greentore
Copy link
Copy Markdown
Contributor Author

@williamkray looks like I broke it again. Thanks for the report.

@williamkray
Copy link
Copy Markdown

looks good after the fix!

@greentore greentore marked this pull request as draft August 7, 2024 22:07
Copy link
Copy Markdown
Member

@ajbura ajbura left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it looks great to me.

Could you please mark the PR as ready for review.

Comment thread src/app/components/message/Reply.css.ts Outdated
@greentore greentore marked this pull request as ready for review August 14, 2024 17:59
@greentore
Copy link
Copy Markdown
Contributor Author

I converted rem units to equivalent toRem calls as requested.
I also considered disabling thread support in sdk, but resulting unthreaded read receipts mark all threads in the room as read in threaded clients, which may be considered undesired behavior.

@ajbura ajbura merged commit 830d05e into cinnyapp:dev Aug 15, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Threads

5 participants