Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Ask to refresh timeline when historical messages are imported (MSC2716) #8354

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Apr 19, 2022

Ask to refresh timeline when historical messages are imported (MSC2716)

Associated matrix-js-sdk PR: matrix-org/matrix-js-sdk#2299

Loading state:

Error state for network errors (no need to submit debug logs):

Error state for other errors you can submit debug logs:

Beware: edge case

There is one edge case where the "History import detected" banner can re-appear after you have refreshed the timeline. matrix-js-sdk only persists /sync data to IndexDB every 5 minutes which means it can have a stale next_batch pagination token compared to the latest events you might be seeing on screen. So if you receive a marker event, it will show the banner to refresh the timeline. You can then use the button to refresh your timeline and the banner will disappear as expected. Then if you restart your entire Element client (like refreshing the entire browser), Element will syncFromCache with the /sync response from 5 minutes ago and query /sync with that old pagination token which will fetch the marker event again and show the banner again.

Why are we just setting timlineNeedsRefresh (and prompting the user) instead of automatically refreshing the timeline for the user?

If we refreshed the timeline automatically, someone could cause your Element client to constantly refresh the timeline by just sending marker events over and over. Granted, you probably want to leave a room like this 🤷. Perhaps also some sort of DOS vector since everyone will be refreshing and hitting the server at the exact same time.

-- matrix-org/matrix-js-sdk#2299

Testing strategy

  1. In your Synapse homeserver.yaml, enable experimental_features -> msc2716_enabled: true
  2. Have the room open in Element with this PR checked out locally
  3. Create a room with a MSC2716 room version, POST http://localhost:8008/_matrix/client/v3/createRoom with
    {
        "preset":       "public_chat",
        "name":         "test-msc2716",
        "room_version": "org.matrix.msc2716v3",
        "invite": ["@erictroupetester:my.synapse.server"]
    }
  4. Send some messages in the room just so we have something to import in the middle of. Let's just say you sent 10 messages (1 though 10)
  5. Pick one of those previous messages (lets just pick 5) and use it in the ?prev_event_id parameter for the batch send request: POST http://localhost:8008/_matrix/client/unstable/org.matrix.msc2716/rooms/{roomId}/batch_send?prev_event_id={someEventId} with
    {
      "state_events_at_start": [
        {
          "content": {
            "displayname": "some-display-name-for-@maria-01234:my.synapse.server",
            "membership": "join"
          },
          "origin_server_ts": 1642055555924,
          "sender": "@maria-01234:my.synapse.server",
          "state_key": "@maria-01234:my.synapse.server",
          "type": "m.room.member"
        }
      ],
      "events": [
        {
          "content": {
            "body": "Historical 0 (batch=0)",
            "msgtype": "m.text",
            "org.matrix.msc2716.historical": true
          },
          "origin_server_ts": 1642055555924,
          "sender": "@maria-01234:my.synapse.server",
          "type": "m.room.message"
        },
        {
          "content": {
            "body": "Historical 1 (batch=0)",
            "msgtype": "m.text",
            "org.matrix.msc2716.historical": true
          },
          "origin_server_ts": 1642055555925,
          "sender": "@maria-01234:my.synapse.server",
          "type": "m.room.message"
        },
        {
          "content": {
            "body": "Historical 2 (batch=0)",
            "msgtype": "m.text",
            "org.matrix.msc2716.historical": true
          },
          "origin_server_ts": 1642055555926,
          "sender": "@maria-01234:my.synapse.server",
          "type": "m.room.message"
        },
        {
          "content": {
            "body": "Historical 3 (batch=0)",
            "msgtype": "m.text",
            "org.matrix.msc2716.historical": true
          },
          "origin_server_ts": 1642055555927,
          "sender": "@maria-01234:my.synapse.server",
          "type": "m.room.message"
        }
      ]
    }
  6. Send a marker event in the room to expose the history to everyone. Use the base_insertion_event_id from the batch send response in the org.matrix.msc2716.marker.insertion field in the JSON body: PUT http://localhost:8008/_matrix/client/r0/rooms/{roomId}/state/org.matrix.msc2716.marker
    {
      "org.matrix.msc2716.marker.insertion": "$jxIsfEXmnUtl6KroclFmselixa1FEzQfBgKWlqV3Bfg",
    }
  7. Notice the history import detected banner
  8. Press the Refresh timeline button
  9. Notice the room timeline refreshes and now you can see the historical messages in the room

Dev notes

First reviewed in #8303

loadTimeline
refreshTimeline
onRoomTimelineReset

onRoomTimeline
onMessageListFillRequest
onMessageListUnfillRequest

Generic apply data-xxx attributes to to child components (based off of https://stackoverflow.com/a/65656589/796832):

function getDataAttributes(inputProps = {}) {
    return Object.keys(inputProps).reduce((dataAttrs, key) => {
        if (key.startsWith('data-')) {
            dataAttrs[key] = inputProps[key];
        }

        return dataAttrs;
    }, {});
}


<SomeJSXComponent {...getDataAttributes(restProps)}>

Cypress

Clean up Synapse Docker instances from Cypress

docker ps --filter "name=react-sdk-cypress-synapse-*" -aq | xargs docker stop
docker ps --filter "name=react-sdk-cypress-synapse-*" -aq | xargs docker rm

Can't queue within cy.intercept callbacks

Error:

Cypress detected that you returned a promise from a command while also invoking one or more cy commands in that promise.

Snippet without async still fails ❌

cy.intercept(contextUrl, (req) => {
    console.log('intercepted');
    cy.get('.mx_BasicMessageComposer').click()

    // Now continue the request
    req.reply();
}).as('contextRequestThatWillMakeNewTimeline');

Snippet with async use case fails too ❌

cy.intercept(contextUrl, async (req) => {
    console.log('intercepted');
    const {event_id: eventIdWhileRefrshingTimeline } = await asMatrixClient.sendMessage(roomId, null, {
        body: `live_event while trying to refresh timeline`,
        msgtype: "m.text",
    });
    
    // Wait for the message to show up for the logged in user
    // indicating that a sync happened
    waitForEventIdsInClient([eventIdWhileRefrshingTimeline]);

    // Now continue the request
    req.continue();
    //req.reply();
}).as('contextRequestThatWillMakeNewTimeline');

Related issues:

cy.intercept(..., { forceNetworkError: true }) does not play well with multiple matching requests or overriding

cy.intercept() routes are matched in reverse order of definition, except for routes which are defined with { middleware: true }, which always run first. This allows you to override existing cy.intercept() declarations by defining an overlapping cy.intercept():

https://docs.cypress.io/api/commands/intercept#Interception-lifecycle

But if you try to override a cy.intercept that uses forceNetworkError, the second match will always fail. Both of the following examples work if you just switch it out for { statusCode: 500 } instead of the network failure

Snippet trying to override intercepts ❌

cy.intercept('/context', { times: 1 }, { forceNetworkError: true }).as('contextRequestThatWillTryToMakeNewTimeline');
cy.wait('@contextRequestThatWillMakeNewTimeline').should('have.property', 'error');

// This will fail on two different fronts:
//   1. This won't have a "response" because the first intercept is overriding
//   2. Not specifying a StaticResponse will cause it to be overriden as well even if we use `statusCode: 500` above instead of a network error
cy.intercept('/context').as('contextRequestThatWillMakeNewTimeline');
cy.wait('@contextRequestThatWillMakeNewTimeline').its('response.statusCode').should('eq', 200);

Snippet trying to conditionally fail/passthrough request ❌

let failContextRequests = true;
cy.wrap(null).then(function() {
    cy.intercept('/context', async (req) => {
        // Force a network error the first time so we have to retry
        console.log('intercept', failContextRequests);
        if (failContextRequests) {
            console.log('intercept error');
            req.reply({ forceNetworkError: true });
        } else {
            // Otherwise, respond as normal
            console.log('intercept normal');
            req.reply();
        }
    }).as('contextRequestThatWillMakeNewTimeline');
});

// Press "Refresh timeline"
cy.get(`[data-cy="refresh-timeline-button"]`).click();

// Make sure the request was intercepted and thew an network error
cy.wait('@contextRequestThatWillMakeNewTimeline').should('have.property', 'error');

// Make sure we tell the user that an error happened
cy.get(`[data-cy="historical-import-detected-error-content"]`).should("exist");

// Allow the requests to succeed now
cy.wrap(null).then(() => {
    failContextRequests = false;
});

// Press "Refresh timeline" again, this time the network request should succeed
cy.get(`[data-cy="refresh-timeline-button"]`).click();

// Make sure the request was intercepted and succeeded
// (this will fail with never having a response)
cy.wait('@contextRequestThatWillMakeNewTimeline').its('response.statusCode').should('eq', 200);

Related issues:

Debugging Synapse in the Cypress e2e tests

In cypress/plugins/synapsedocker/index.ts#L124, change the image that the synapsedocker Cypress plugin is using to spin up Synapse instances to matrixdotorg/synapse:latest. Restart your Cypress process.

In your local Synapse checkout, make any changes and build the image: docker build -t matrixdotorg/synapse -f docker/Dockerfile .


Logging code in Synapse to generate the tabulated events from the room you're interested in:

logger.info(
    "asdf_get_debug_events_in_room_ordered_by_depth\n%s",
    await self.store.asdf_get_debug_events_in_room_ordered_by_depth(room_id),
)
async def asdf_get_debug_events_in_room_ordered_by_depth(self, room_id: str) -> Any:
    """Gets the topological token in a room after or at the given stream
    ordering.
    Args:
        room_id
    """
    sql = (
        "SELECT depth, stream_ordering, type, state_key, event_id FROM events"
        " WHERE events.room_id = ?"
        " ORDER BY depth DESC, stream_ordering DESC;"
    )
    rows = await self.db_pool.execute(
        "asdf_get_debug_events_in_room_ordered_by_depth", None, sql, room_id
    )

    headerColumnLengthMap = {
        "depth": 4,
        "stream_ordering": 12,
        "type": 30,
        "state_key": 28,
        # event_ids are 44 long but we don't need the extra 5 padding
        # because it's the last item and we're just cheating by building
        # this into the value instead of the format code.
        "event_id": 39,
    }

    output = ""
    row_format = "".join(
        [
            "{:<" + str(columnLength + 5) + "}"
            for columnLength in headerColumnLengthMap.values()
        ]
    )
    output += row_format.format(*headerColumnLengthMap.keys()) + "\n"
    for row in rows:
        output += row_format.format(*[str(x) for x in row]) + "\n"
    return output

Example output:

depth     stream_ordering     type                               state_key                          event_id                                         
12        13                  org.matrix.msc2716.marker          marker_state_key_2087              $bokn_JFlThduvpLCDQcbzSo3UUIdzM3s3ldS5VdFyYQ     
11        12                  m.room.message                     None                               $U0_8sLrLF3pqcKnEg-3ewUbzyfHDS1EB6YfKVh-ZnQc     
10        11                  m.room.message                     None                               $0CIrzvW7ZTh0x4gtI5d3mXyDXvt1kuCTb7iISfNtHlQ     
10        -3                  org.matrix.msc2716.insertion       None                               $dWZvpd2rEuImJdZ4aSk97Dj8cYXR-b2xMUMWG7FYm6E     
10        -4                  org.matrix.msc2716.batch           None                               $h1deiGVBrEV8QCsAwbwxHkjFO-sHdwvP251N8GYaYIw     
10        -5                  m.room.message                     None                               $BrgyUUoD8ITK3BRaOGPUGMWiH78dhs9FrjCwH0wgwfI     
10        -6                  m.room.message                     None                               $1OOGZAlyTehMK7p3mJhVXRvs7qnJ4MRXtBMFHTONLHw     
10        -7                  m.room.message                     None                               $drnLdMjvo5e_NsfqBGVsPQcejyOZvbTrNXKvD2KZ_QE     
10        -8                  org.matrix.msc2716.insertion       None                               $T6u8-XmI-ZFAnbxHx1HFXXTgCJ82lVtd71oulCc0Uo8     
9         10                  m.room.message                     None                               $Cu7LaaoGrCMnoXPn7uVsHWJ5W-XG4WSG-MiBWlY24z0     
8         9                   m.room.message                     None                               $AkJKzohur-0DNkrd6w3u9hzvdzYFIeEX31a1IwX7evo     
7         8                   m.room.member                      @userid_1961:localhost             $jw6pB2SEBqlOhKiwIrUsGiH79aRHLGum1pJVY-t3L4o     
6         7                   m.room.name                                                           $3krkuB2G3MrzEBS3zGRvldzYpGAGsLmoIuMPvXXZE2w     
5         6                   m.room.history_visibility                                             $pNca6LVT6x6vtG4AITsbMrxuO0AoEdnNsDb3jbFsCNw     
4         5                   m.room.join_rules                                                     $BYCJnsez0Ic1ZGMSvZa2NwLjZIigI_A-2amN3JkTkEY     
3         4                   m.room.power_levels                                                   $5D5WzmPfY2wwVu0YU6A815h6bw4Pb1L5NLlSbq9z-Og     
2         3                   m.room.member                      @gitter-badger:localhost           $0aP1Sh_mIB4eLnA2vXaCfwS8wSNAWyugIo3N_yFYevc     
1         2                   m.room.create                                                         $dNbk-MSdtXsPH7PYr3q6ZWy1z87B4wrMJ6SSf2rtvm8     
1         -2                  m.room.member                      @maria-01234:localhost             $Xz7QxLkgDRtaTOse1oAw0wpFBmM58Oqdkj08Q3wGQ5k     

Todo


Here's what your changelog entry will look like:

✨ Features

Preview: https://pr8354--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@MadLittleMods MadLittleMods added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Apr 19, 2022
@MadLittleMods MadLittleMods requested a review from a team as a code owner April 19, 2022 04:31
@MadLittleMods MadLittleMods marked this pull request as draft April 19, 2022 04:31
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

clearing code review

@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #8354 (f571107) into develop (27118a9) will increase coverage by 0.07%.
The diff coverage is 40.62%.

@@             Coverage Diff             @@
##           develop    #8354      +/-   ##
===========================================
+ Coverage    30.01%   30.09%   +0.07%     
===========================================
  Files          879      879              
  Lines        50193    50220      +27     
  Branches     12783    12791       +8     
===========================================
+ Hits         15064    15112      +48     
+ Misses       35129    35108      -21     
Impacted Files Coverage Δ
src/components/structures/FilePanel.tsx 1.12% <0.00%> (ø)
src/components/structures/TimelinePanel.tsx 1.63% <0.00%> (-0.03%) ⬇️
src/indexing/EventIndex.ts 0.59% <0.00%> (ø)
src/components/structures/RoomStatusBar.tsx 52.88% <72.22%> (+44.93%) ⬆️

if (timelineSet !== this.props.timelineSet) return;

if (this.messagePanel.current && this.messagePanel.current.isAtBottom()) {
if (this.canResetTimeline()) {
Copy link
Contributor Author

@MadLittleMods MadLittleMods Apr 19, 2022

Choose a reason for hiding this comment

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

This seems like a missed refactor. canResetTimeline by name seems applicable here and the logic appears pretty equivalent.

public canResetTimeline = () => {
if (!this.messagePanel) {
return true;
}
return this.messagePanel.canResetTimeline();
};

public canResetTimeline = () => this.messagePanel?.current.isAtBottom();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split out to #10403

@MadLittleMods MadLittleMods requested a review from t3chguy April 19, 2022 07:36
@MadLittleMods MadLittleMods marked this pull request as ready for review April 19, 2022 07:36
See:
```
Error: src/utils/ErrorUtils.tsx(39,5): error TS1016: A required parameter cannot follow an optional parameter.
```
…msc2716-marker-events-v2

Conflicts:
	src/components/structures/MessagePanel.tsx
	src/components/views/rooms/EventTile.tsx
	src/i18n/strings/en_EN.json
	test/components/structures/__snapshots__/RoomView-test.tsx.snap
@andybalaam
Copy link
Member

What's the state of this change? It looks like a huge amount of work has gone into it, and it's been around for ~6 months!

@MadLittleMods
Copy link
Contributor Author

@andybalaam The implementation on this side has been ready to go since April 20th when it had the review pass. And I've kept it up to date at various points (whenever I merge develop).

It's been blocked behind the refresh timeline function in the matrix-js-sdk which has an edge case with threads. I initially fixed this in a v1 fix but it didn't pass review (see comments there) and now a v2 change has been made but still requires more cycles to conform.

I'm focusing on other things so I guess it will continue to languish 🗻

@andybalaam
Copy link
Member

OK so @MadLittleMods it doesn't look like you're blocked on us for anything here - just needing to find time to respond to review requests on matrix-org/matrix-js-sdk#2852 . Let me know if we can help.

MadLittleMods added a commit that referenced this pull request Mar 17, 2023
MadLittleMods added a commit that referenced this pull request Mar 20, 2023
… (nonsense to screenreaders) (#10402)

And the other content like the title already describe what's going on sufficiently.

Split out from #8354
// The submit debug logs option should *NOT* be shown.
//
// We have to use `queryBy` so that it can return `null` for something that does not exist.
expect(wrapper.queryByTestId('historical-import-detected-error-submit-debug-logs-button]')).toBeNull();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
expect(wrapper.queryByTestId('historical-import-detected-error-submit-debug-logs-button]')).toBeNull();
expect(wrapper.queryByTestId('historical-import-detected-error-submit-debug-logs-button')).toBeNull();

MadLittleMods added a commit that referenced this pull request Mar 21, 2023
if (this.state.refreshError) {
let errorTextContent;
let submitDebugLogsTextContent;
if (this.state.refreshError.name === "ConnectionError") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the pattern we ended up with in #10405

@MadLittleMods
Copy link
Contributor Author

Abandoning PR as I don't see MSC2716 going further now that Gitter has fully migrated to Matrix

toomore pushed a commit to moda-gov-tw/matrix-org.matrix-react-sdk that referenced this pull request Apr 6, 2023
toomore pushed a commit to moda-gov-tw/matrix-org.matrix-react-sdk that referenced this pull request Apr 6, 2023
… (nonsense to screenreaders) (#10402)

And the other content like the title already describe what's going on sufficiently.

Split out from matrix-org/matrix-react-sdk#8354
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants