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.

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

Reply preview placeholders don't always populate #1286

Open
ara4n opened this issue Jul 8, 2023 · 7 comments
Open

Reply preview placeholders don't always populate #1286

ara4n opened this issue Jul 8, 2023 · 7 comments
Labels
A-Timeline O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect X-Needs-Rust This issue needs a Rust SDK change. It must have a link to a Rust SDK issue

Comments

@ara4n
Copy link
Member

ara4n commented Jul 8, 2023

Steps to reproduce

  1. Open a room
  2. See a reply, with a reply preview placeholder embedded in it.
  3. The placeholder never resolves with a preview of the replied message, despite the user having permission (and keys) to view the reply.
  4. Scroll up the timeline to force the app to load the reply
  5. Observe the placeholder now resolves.

Outcome

What did you expect?

Reliable reply previews

What happened instead?

App doesn't seem to proactively go and fetch reply previews via /context or bundled aggregations or whatever if the timeline doesn't already track the replied msg's event.

Your phone model

No response

Operating system version

No response

Application version

279

Homeserver

No response

Will you send logs?

No

@ara4n ara4n added the T-Defect label Jul 8, 2023
@stefanceriu stefanceriu added S-Major Severely degrades major functionality or product features, with no satisfactory workaround O-Occasional Affects or can be seen by some users regularly or most users rarely labels Jul 19, 2023
@ara4n
Copy link
Member Author

ara4n commented Nov 1, 2023

just caught this one in the act and rageshaked it: https://github.com/vector-im/element-x-ios-rageshakes/issues/1044

@ara4n
Copy link
Member Author

ara4n commented Dec 22, 2023

this still happens once every few days for me:

image

@stefanceriu stefanceriu added the X-Needs-Rust This issue needs a Rust SDK change. It must have a link to a Rust SDK issue label Mar 28, 2024
@giomfo
Copy link
Member

giomfo commented Mar 29, 2024

I don't reproduce this issue in v1.6.2 (559).
I opened several rooms, and succeeded to observe several replies with a reply preview placeholder embedded in them. The placeholder was always resolves with a preview of the replied message.

@pixlwave
Copy link
Member

Closing this as stale, seems to work as expected even when the reply is years old (so not yet downloaded through a pagination)

@pixlwave pixlwave closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2024
@ara4n
Copy link
Member Author

ara4n commented Aug 4, 2024

Still happening on build 667. The reply is stuck in state "pending":

image


EventTimelineItem {
    sender: "@andybalaam:matrix.org",
    sender_profile: Ready(
        Profile {
            display_name: Some(
                "andybalaam",
            ),
            display_name_ambiguous: false,
            avatar_url: Some(
                "mxc://matrix.org/HcOKfHoyUseJyNvJCZbySygK",
            ),
        },
    ),
    timestamp: 2024-07-31T13:33:38.769,
    content: Message(
        Message {
            in_reply_to: Some(
                InReplyToDetails {
                    event_id: "$Gy7wlbILga6N7xOMp9QNgBThOMxIkqG-wBbpaFVmtFQ",
                    event: Pending,
                },
            ),
            thread_root: None,
            edited: false,
            ..
        },
    ),
    kind: Remote(
        RemoteEventTimelineItem {
            event_id: "$Ki2KRc60Uada13bifSqlmyWUWWytN2oxjvTagZYo0Yk",
            transaction_id: None,
            reactions: {},
            read_receipts: {},
            is_own: false,
            is_highlighted: false,
            encryption_info: Some(
                EncryptionInfo {
                    sender: "@andybalaam:matrix.org",
                    sender_device: Some(
                        "MKBDKTQJQJ",
                    ),
                    algorithm_info: MegolmV1AesSha2 {
                        curve25519_key: "Ss6eaJGA82Uc9TQn1BbcLqpm6jtjGGwqMBadf/8Y+WY",
                        sender_claimed_keys: {
                            "ed25519": "mPXKwg8Yz0WIDedksSIbcSdYSyQMB1HI08skehk16tc",
                        },
                    },
                    verification_state: Unverified(
                        None(
                            InsecureSource,
                        ),
                    ),
                },
            ),
            origin: Pagination,
            ..
        },
    ),
}

{
  "sender" : "@andybalaam:matrix.org",
  "content" : {
    "body" : "> <@andybalaam:matrix.org> Hi Element X, we in the Crypto team have one additional piece of work for \"EX production-ready\" - it's the one that was marked as \"not ready\" when we had a conversation a couple of days ago.\n> \n> We now have a preferred option and I wanted to ask you whether it sounds feasible.\n> \n> The purpose is to prevent people who have explicitly verified someone accidentally sending messages. We need to cover 2 cases:\n> \n> 1. A verified person becomes unverified\n> 2. A verified person has a new unsigned device\n> \n> in both cases, when I try to send a message, we want sending to fail, and then an error message to be displayed.\n> \n> So we expect to change the message sending code (in Rust) to throw an error in these cases, and we expect the UI to display the error and offer options.\n> \n> The options in case 1 would be \"withdraw verification\" (mark the person as unverified and send the message) or \"cancel sending\" (discard the message). Note that on EX there is no \"Re-verify\", which is what we would really like.\n> \n> The options in case 2 would be \"send anyway\" (send the message to the unverified device and remember that we don't need to ask again for that device) or \"cancel sending\" (discard the message).\n> \n> I'd like to ask:\n> \n> * in terms of the Rust side: sending messages throwing this kind of permanent error, and the send queue dealing with it\n> * in terms of the UI sides: displaying these errors on send and taking the actions\n> \n> Does this sound do-able?\n\nI made an issue to cover this item: https:\/\/github.com\/element-hq\/element-meta\/issues\/2488",
    "format" : "org.matrix.custom.html",
    "formatted_body" : "<mx-reply><blockquote><a href=\"https:\/\/matrix.to\/#\/!kCCQTCfnABLKGGvQjo:matrix.org\/$Gy7wlbILga6N7xOMp9QNgBThOMxIkqG-wBbpaFVmtFQ?via=matrix.org&via=element.io&via=one.ems.host\">In reply to<\/a> <a href=\"https:\/\/matrix.to\/#\/@andybalaam:matrix.org\">@andybalaam:matrix.org<\/a><br><p>Hi Element X, we in the Crypto team have one additional piece of work for \"EX production-ready\" - it's the one that was marked as \"not ready\" when we had a conversation a couple of days ago.<\/p>\n<p>We now have a preferred option and I wanted to ask you whether it sounds feasible.<\/p>\n<p>The purpose is to prevent people who have explicitly verified someone accidentally sending messages. We need to cover 2 cases:<\/p>\n<ol>\n<li>A verified person becomes unverified<\/li>\n<li>A verified person has a new unsigned device<\/li>\n<\/ol>\n<p>in both cases, when I try to send a message, we want sending to fail, and then an error message to be displayed.<\/p>\n<p>So we expect to change the message sending code (in Rust) to throw an error in these cases, and we expect the UI to display the error and offer options.<\/p>\n<p>The options in case 1 would be \"withdraw verification\" (mark the person as unverified and send the message) or \"cancel sending\" (discard the message). Note that on EX there is no \"Re-verify\", which is what we would really like.<\/p>\n<p>The options in case 2 would be \"send anyway\" (send the message to the unverified device and remember that we don't need to ask again for that device) or \"cancel sending\" (discard the message).<\/p>\n<p>I'd like to ask:<\/p>\n<ul>\n<li>in terms of the Rust side: sending messages throwing this kind of permanent error, and the send queue dealing with it<\/li>\n<li>in terms of the UI sides: displaying these errors on send and taking the actions<\/li>\n<\/ul>\n<p>Does this sound do-able?<\/p>\n<\/blockquote><\/mx-reply>I made an issue to cover this item: https:\/\/github.com\/element-hq\/element-meta\/issues\/2488",
    "m.mentions" : {

    },
    "m.relates_to" : {
      "m.in_reply_to" : {
        "event_id" : "$Gy7wlbILga6N7xOMp9QNgBThOMxIkqG-wBbpaFVmtFQ"
      },
      "is_falling_back" : false
    },
    "msgtype" : "m.text"
  },
  "origin_server_ts" : 1722432818769,
  "room_id" : "!kCCQTCfnABLKGGvQjo:matrix.org",
  "event_id" : "$Ki2KRc60Uada13bifSqlmyWUWWytN2oxjvTagZYo0Yk",
  "type" : "m.room.message",
  "unsigned" : {
    "age" : 344382435,
    "membership" : "join"
  }
}

See also #3113

@ara4n
Copy link
Member Author

ara4n commented Aug 9, 2024

just got it again. it happens if the most recent msg in the timeline is a reply, but the other history in the timeline hasn't loaded yet. then, when /messages (or whatever) pulls in the remaining timeline, the reply preview doesn't get rerendered.

bging and then fging the app is enough to then kick a rerender.

@ara4n
Copy link
Member Author

ara4n commented Sep 30, 2024

still happening today - and on EXA too:

Screenshot 2024-09-30 at 14 05 36

^ for instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Timeline O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect X-Needs-Rust This issue needs a Rust SDK change. It must have a link to a Rust SDK issue
Projects
None yet
Development

No branches or pull requests

5 participants