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.

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

Jump to bottom

Fix edge cases around non-thread relations to thread roots and read receipts #3607

Merged
merged 8 commits into from
Jul 19, 2023
Merged

Fix edge cases around non-thread relations to thread roots and read receipts #3607

merged 8 commits into from
Jul 19, 2023

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Jul 18, 2023 •
edited by github-actions bot
Loading

Which caused stuck notifications

Requires matrix-org/matrix-react-sdk#11294

Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fix edge cases around non-thread relations to thread roots and read receipts (#3607).

@t3chguy t3chguy mentioned this pull request Jul 18, 2023
Closed
@t3chguy t3chguy marked this pull request as ready for review July 18, 2023 10:50
@t3chguy t3chguy requested a review from a team as a code owner July 18, 2023 10:50
@t3chguy t3chguy requested review from germain-gg and dbkr July 18, 2023 10:50
andybalaam
andybalaam approved these changes Jul 18, 2023
View reviewed changes
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks good, one tiny comment.

spec/unit/read-receipt.spec.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
t3chguy
t3chguy commented Jul 18, 2023
View reviewed changes
spec/unit/read-receipt.spec.ts Outdated Show resolved Hide resolved
spec/unit/read-receipt.spec.ts Outdated Show resolved Hide resolved
@t3chguy t3chguy marked this pull request as draft July 18, 2023 11:40
@t3chguy t3chguy changed the title Fixes around thread root non-thread relations and read receipts Fix edge cases around non-thread relations to thread roots and read receipts Jul 18, 2023
@t3chguy t3chguy requested a review from andybalaam July 18, 2023 16:02
@t3chguy t3chguy marked this pull request as ready for review July 18, 2023 16:02
@t3chguy t3chguy mentioned this pull request Jul 18, 2023
Merged
andybalaam
andybalaam requested changes Jul 19, 2023
View reviewed changes
Copy link
Member

@andybalaam andybalaam left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I just tried this out locally, and edits to thread roots are not reflected until I refresh:

image

(Changing to a different room and back did not fix the problem)

Switching back to develop, I see the change immediately.

@t3chguy
Copy link
Member Author

t3chguy commented Jul 19, 2023

Screen.Recording.2023-07-19.at.11.46.07.mov

@andybalaam I cannot reproduce that at all, are you sure your build was fine? I'm using https://pr11294--matrix-react-sdk.netlify.app/

@andybalaam
Copy link
Member

Screen.Recording.2023-07-19.at.11.46.07.mov

@andybalaam I cannot reproduce that at all, are you sure your build was fine? I'm using https://pr11294--matrix-react-sdk.netlify.app/

On the netlify build I see the same as you, but when I build locally, with only matrix-js-sdk on t3chguy/stuck-edits-thread-root I see the behaviour I described.

I note that there is still a t3chguy/stuck-edits-thread-root branch on matrix-react-sdk even though the PR is closed: matrix-org/matrix-react-sdk#11291

Could branch matching be fooling us here?

@t3chguy
Copy link
Member Author

t3chguy commented Jul 19, 2023

https://pr11294--matrix-react-sdk.netlify.app/ is a build from matrix-org/matrix-react-sdk#11294 not matrix-org/matrix-react-sdk#11291 - the branch was reused between PRs and has since been deleted. Given matrix-org/matrix-react-sdk#11294 only touches cypress I don't see how branch matching could be doing anything awry here. Critically in the screencap I included above you can see the edit hidden event is in the main timeline, unlike on develop where it'd be in the thread timeline:

image

andybalaam
andybalaam approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

OK, I just rebuilt locally and it worked fine, so I have no idea what was happening before - I would doubt it happened, except the screenshot shows something.

Good that this is going in now, so we've some time to test before the next RC, but it looks sensible to me, thanks!

@t3chguy t3chguy added this pull request to the merge queue Jul 19, 2023
Merged via the queue into develop with commit 66492e7 Jul 19, 2023
@t3chguy t3chguy deleted the t3chguy/stuck-edits-thread-root branch July 19, 2023 11:38
@Johennes Johennes mentioned this pull request Jul 24, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andybalaam andybalaam andybalaam approved these changes

@germain-gg germain-gg Awaiting requested review from germain-gg germain-gg is a code owner automatically assigned from matrix-org/element-web-reviewers

@dbkr dbkr Awaiting requested review from dbkr dbkr is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Defect
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@t3chguy @andybalaam