Extra insurance that we don't mix events in the wrong timelines - v2 #2856

MadLittleMods · 2022-11-04T17:33:06Z

Add checks to addEventToTimeline as extra insurance that we don't mix events in the wrong timelines (main timeline vs thread timeline).

Split out from #2521

This PR is a v2 of #2848 since it was reverted in #2853

Previously, we just relied on the callers to make sure they're doing the right thing and since it's easy to get it wrong, we mixed and bugs happened.

Dev notes

TimelineSet.addEventsToTimeline -> TimelineSet.addEventToTimeline -> Timeline.addEvent
TimelineSet.addEventToTimeline -> Timeline.addEvent
TimelineSet.addLiveEvent -> TimelineSet.addEventToTimeline -> Timeline.addEvent

yarn jest spec/unit/event-timeline-set.spec.ts

Todo

Ensure the matrix-react-sdk Cypress tests pass with these changes
Ensure the matrix-react-sdk, yarn jest test/components/structures/ThreadView-test.tsx pass with these changes
Ensure the matrix-react-sdk, yarn jest test/Notifier-test.ts pass with these changes

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
~~Sign-off given on the changes (see CONTRIBUTING.md)~~

Here's what your changelog entry will look like:

🐛 Bug Fixes

Extra insurance that we don't mix events in the wrong timelines - v2 (#2856). Contributed by @MadLittleMods.

Split out from #2521

…rding to the signature

Fix test failure in `sliding-sync/sliding-sync.ts` -> `"should show the right unread notifications"` ``` Cannot call `EventTimelineSet::canContain without a `room` set. Set the room when creating the EventTimelineSet to call this method. at EventTimelineSet.canContain (VM4503 200:726:13) at EventTimelineSet.addEventToTimeline (VM4503 200:576:15) at EventTimelineSet.addLiveEvent (VM4503 200:527:10) at eval (VM4587 281:722:145) at Array.forEach (<anonymous>) at SlidingSyncSdk.purgeNotifications (VM4587 281:720:22) at SlidingSyncSdk.onLifecycle (VM4587 281:263:14) at SlidingSync.emit (VM4407 21:153:5) at SlidingSync.emit (VM4472 169:45:18) at SlidingSync.invokeLifecycleListeners (VM4588 282:378:10) ```

MadLittleMods · 2022-11-04T22:18:01Z

src/models/event-timeline-set.ts

+        //
+        // We can only run this check for timelines with a `room` because `canContain`
+        // requires it
+        if (this.room && !this.canContain(event)) {


Main change from v1 is here. We added the extra this.room check as canContain requires it

MadLittleMods · 2022-11-04T22:18:21Z

spec/unit/event-timeline-set.spec.ts

+        describe('non-room timeline', () => {
+            fit('Adds event to timeline', () => {
+                const nonRoomEventTimelineSet = new EventTimelineSet(
+                    // This is what we're specifically testing against, a timeline
+                    // without a `room` defined
+                    undefined,
+                );
+                const nonRoomEventTimeline = new EventTimeline(nonRoomEventTimelineSet);
+
+                expect(nonRoomEventTimeline.getEvents().length).toStrictEqual(0);
+                nonRoomEventTimelineSet.addEventToTimeline(messageEvent, nonRoomEventTimeline, {
+                    toStartOfTimeline: true,
+                });
+                expect(nonRoomEventTimeline.getEvents().length).toStrictEqual(1);
+            });
+        });


Added a test for a non-room timeline scenario that would fail without the additional this.room check added in this PR compared to v1.

See https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&types=CODE_SMELL&pullRequest=2856&id=matrix-js-sdk&open=AYRElaAdNY5nTGOdJo3a

src/models/event-timeline-set.ts

spec/unit/event-timeline-set.spec.ts

Relevant rule: https://github.com/jest-community/eslint-plugin-jest/blob/ad04fcc97b5dcc676c5bd2152f84608c08e0c50a/docs/rules/no-focused-tests.md Spawning from #2856 (comment)

t3chguy · 2022-11-07T17:54:49Z

@MadLittleMods I suggest waiting on the auto assigned reviewers, if you're waiting on me it'll be end of week due to the fires I'm dealing with

robintown

The main substance of the change looks good

src/models/room.ts

src/models/event.ts

Fixes tests failing from changes made by matrix-org/matrix-js-sdk#2856

MadLittleMods · 2022-11-07T20:25:03Z

Thanks for the review @t3chguy and @robintown 🐄

@MrAnno

* Make calls go back to 'connecting' state when media lost ([\matrix-org#2880](matrix-org#2880)). * Add ability to send unthreaded receipt ([\matrix-org#2878](matrix-org#2878)). * Add way to abort search requests ([\matrix-org#2877](matrix-org#2877)). * sliding sync: add custom room subscriptions support ([\matrix-org#2834](matrix-org#2834)). * webrtc: add advanced audio settings ([\matrix-org#2434](matrix-org#2434)). Contributed by @MrAnno. * Add support for group calls using MSC3401 ([\matrix-org#2553](matrix-org#2553)). * Make the js-sdk conform to tsc --strict ([\matrix-org#2835](matrix-org#2835)). Fixes matrix-org#2112 matrix-org#2116 and matrix-org#2124. * Let leave requests outlive the window ([\matrix-org#2815](matrix-org#2815)). Fixes element-hq/element-call#639. * Add event and message capabilities to RoomWidgetClient ([\matrix-org#2797](matrix-org#2797)). * Misc fixes for group call widgets ([\matrix-org#2657](matrix-org#2657)). * Support nested Matrix clients via the widget API ([\matrix-org#2473](matrix-org#2473)). * Set max average bitrate on PTT calls ([\matrix-org#2499](matrix-org#2499)). Fixes element-hq/element-call#440. * Add config option for e2e group call signalling ([\matrix-org#2492](matrix-org#2492)). * Enable DTX on audio tracks in calls ([\matrix-org#2482](matrix-org#2482)). * Don't ignore call member events with a distant future expiration date ([\matrix-org#2466](matrix-org#2466)). * Expire call member state events after 1 hour ([\matrix-org#2446](matrix-org#2446)). * Emit unknown device errors for group call participants without e2e ([\matrix-org#2447](matrix-org#2447)). * Mute disconnected peers in PTT mode ([\matrix-org#2421](matrix-org#2421)). * Add support for sending encrypted to-device events with OLM ([\matrix-org#2322](matrix-org#2322)). Contributed by @robertlong. * Support for PTT group call mode ([\matrix-org#2338](matrix-org#2338)). * Fix registration add phone number not working ([\matrix-org#2876](matrix-org#2876)). Contributed by @bagvand. * Use an underride rule for Element Call notifications ([\matrix-org#2873](matrix-org#2873)). Fixes element-hq/element-web#23691. * Fixes unwanted highlight notifications with encrypted threads ([\matrix-org#2862](matrix-org#2862)). * Extra insurance that we don't mix events in the wrong timelines - v2 ([\matrix-org#2856](matrix-org#2856)). Contributed by @MadLittleMods. * Hide pending events in thread timelines ([\matrix-org#2843](matrix-org#2843)). Fixes element-hq/element-web#23684. * Fix pagination token tracking for mixed room timelines ([\matrix-org#2855](matrix-org#2855)). Fixes element-hq/element-web#23695. * Extra insurance that we don't mix events in the wrong timelines ([\matrix-org#2848](matrix-org#2848)). Contributed by @MadLittleMods. * Do not freeze state in `initialiseState()` ([\matrix-org#2846](matrix-org#2846)). * Don't remove our own member for a split second when entering a call ([\matrix-org#2844](matrix-org#2844)). * Resolve races between `initLocalCallFeed` and `leave` ([\matrix-org#2826](matrix-org#2826)). * Add throwOnFail to groupCall.setScreensharingEnabled ([\matrix-org#2787](matrix-org#2787)). * Fix connectivity regressions ([\matrix-org#2780](matrix-org#2780)). * Fix screenshare failing after several attempts ([\matrix-org#2771](matrix-org#2771)). Fixes element-hq/element-call#625. * Don't block muting/unmuting on network requests ([\matrix-org#2754](matrix-org#2754)). Fixes element-hq/element-call#592. * Fix ICE restarts ([\matrix-org#2702](matrix-org#2702)). * Target widget actions at a specific room ([\matrix-org#2670](matrix-org#2670)). * Add tests for ice candidate sending ([\matrix-org#2674](matrix-org#2674)). * Prevent exception when muting ([\matrix-org#2667](matrix-org#2667)). Fixes element-hq/element-call#578. * Fix race in creating calls ([\matrix-org#2662](matrix-org#2662)). * Add client.waitUntilRoomReadyForGroupCalls() ([\matrix-org#2641](matrix-org#2641)). * Wait for client to start syncing before making group calls ([\matrix-org#2632](matrix-org#2632)). Fixes matrix-org#2589. * Add GroupCallEventHandlerEvent.Room ([\matrix-org#2631](matrix-org#2631)). * Add missing events from reemitter to GroupCall ([\matrix-org#2527](matrix-org#2527)). Contributed by @toger5. * Prevent double mute status changed events ([\matrix-org#2502](matrix-org#2502)). * Don't mute the remote side immediately in PTT calls ([\matrix-org#2487](matrix-org#2487)). Fixes element-hq/element-call#425. * Fix some MatrixCall leaks and use a shared AudioContext ([\matrix-org#2484](matrix-org#2484)). Fixes element-hq/element-call#412. * Don't block muting on determining whether the device exists ([\matrix-org#2461](matrix-org#2461)). * Only clone streams on Safari ([\matrix-org#2450](matrix-org#2450)). Fixes element-hq/element-call#267. * Set PTT mode on call correctly ([\matrix-org#2445](matrix-org#2445)). Fixes element-hq/element-call#382. * Wait for mute event to send in PTT mode ([\matrix-org#2401](matrix-org#2401)). * Handle other members having no e2e keys ([\matrix-org#2383](matrix-org#2383)). Fixes element-hq/element-call#338. * Fix races when muting/unmuting ([\matrix-org#2370](matrix-org#2370)).

MadLittleMods added 3 commits November 3, 2022 19:02

Extra insurance that we don't mix events in the wrong timelines

0f0e8f9

Split out from #2521

Fix null client error in tests even though it should not be null acco…

58ed2f3

…rding to the signature

Content is not required

b91b03b

MadLittleMods added the T-Defect label Nov 4, 2022

MadLittleMods added 3 commits November 4, 2022 13:20

Make sure to remove macOS garbage

7fd36a4

More info in error

a1449f9

MadLittleMods mentioned this pull request Nov 4, 2022

Fix ThreadView tests not using thread flag matrix-org/matrix-react-sdk#9547

Merged

3 tasks

Add test that would fail in the JS sdk

3109e28

MadLittleMods commented Nov 4, 2022

View reviewed changes

SonarQube: Refactor this code to not use nested template literals.

f2f37e6

See https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&types=CODE_SMELL&pullRequest=2856&id=matrix-js-sdk&open=AYRElaAdNY5nTGOdJo3a

MadLittleMods commented Nov 4, 2022

View reviewed changes

src/models/event-timeline-set.ts Show resolved Hide resolved

MadLittleMods marked this pull request as ready for review November 4, 2022 22:41

MadLittleMods requested a review from a team as a code owner November 4, 2022 22:41

MadLittleMods requested review from germain-gg and robintown November 4, 2022 22:41

t3chguy reviewed Nov 7, 2022

View reviewed changes

spec/unit/event-timeline-set.spec.ts Outdated Show resolved Hide resolved

MadLittleMods and others added 2 commits November 7, 2022 11:08

Remove fit

ded1433

Merge branch 'develop' into madlittlemods/no-addEventToTimeline-mixing

e3b6f4f

MadLittleMods mentioned this pull request Nov 7, 2022

Add lint to remind you not to leave an focused/exclusive test in #2859

Closed

3 tasks

MadLittleMods requested a review from t3chguy November 7, 2022 17:51

MadLittleMods removed the request for review from t3chguy November 7, 2022 18:01

robintown reviewed Nov 7, 2022

View reviewed changes

src/models/room.ts Show resolved Hide resolved

src/models/event.ts Show resolved Hide resolved

robintown removed the request for review from germain-gg November 7, 2022 18:09

MadLittleMods requested a review from robintown November 7, 2022 18:18

robintown approved these changes Nov 7, 2022

View reviewed changes

MadLittleMods added a commit to matrix-org/matrix-react-sdk that referenced this pull request Nov 7, 2022

Fix ThreadView tests not using thread flag (#9547)

3cbd88c

Fixes tests failing from changes made by matrix-org/matrix-js-sdk#2856

MadLittleMods merged commit 1646ea0 into develop Nov 7, 2022

MadLittleMods deleted the madlittlemods/no-addEventToTimeline-mixing branch November 7, 2022 20:24

MadLittleMods mentioned this pull request Nov 9, 2022

Add Jest linting - Add lint to remind you not to leave in an exclusive/focused test matrix-org/eslint-plugin-matrix-org#33

Merged

germain-gg mentioned this pull request Dec 21, 2022

Stuck notification in thread element-hq/element-web#23921

Closed

Morris05 mentioned this pull request Jan 10, 2023

Adding the lint via https://github.com/matrix-org/eslint-plugin-matrix-org/pull/33 #3041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extra insurance that we don't mix events in the wrong timelines - v2 #2856

Extra insurance that we don't mix events in the wrong timelines - v2 #2856

MadLittleMods commented Nov 4, 2022 •

edited by github-actions bot

Loading

MadLittleMods Nov 4, 2022 •

edited

Loading

MadLittleMods Nov 4, 2022 •

edited

Loading

t3chguy commented Nov 7, 2022

robintown left a comment

MadLittleMods commented Nov 7, 2022

Extra insurance that we don't mix events in the wrong timelines - v2 #2856

Extra insurance that we don't mix events in the wrong timelines - v2 #2856

Conversation

MadLittleMods commented Nov 4, 2022 • edited by github-actions bot Loading

Dev notes

Todo

Checklist

🐛 Bug Fixes

MadLittleMods Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

t3chguy commented Nov 7, 2022

robintown left a comment

Choose a reason for hiding this comment

MadLittleMods commented Nov 7, 2022

MadLittleMods commented Nov 4, 2022 •

edited by github-actions bot

Loading

MadLittleMods Nov 4, 2022 •

edited

Loading

MadLittleMods Nov 4, 2022 •

edited

Loading