diff --git a/src/views/messages-view.jsx b/src/views/messages-view.jsx index 2fc40dde..d4b919dd 100644 --- a/src/views/messages-view.jsx +++ b/src/views/messages-view.jsx @@ -665,7 +665,7 @@ class MessagesView extends React.Component { } subscribe(topic) { - if (topic.isSubscribed() || !this.props.ready) { + if (!this.props.ready) { return; } @@ -676,7 +676,24 @@ class MessagesView extends React.Component { let getQuery = topic.startMetaQuery().withLaterDesc().withLaterSub().withAux(); if (this.state.isReader || newTopic) { // Reading is either permitted or we don't know because it's a new topic. Ask for messages. - getQuery = getQuery.withLaterData(MESSAGES_PAGE); + // + // withData(undefined, undefined, MESSAGES_PAGE) instead of the seemingly-more-natural + // withLaterData(MESSAGES_PAGE): the latter asks the server for "whatever is newer than + // topic._maxSeq", which is only correct when _maxSeq reflects a real, contiguous, locally-cached + // history. But tinode-web.jsx's prefetchLatestMessagePreviews() (conversation list "last + // message" preview) may have already called topic.subscribe(startMetaQuery().withLaterData(1)) + // for this exact topic the moment the chat list rendered -- often *before* this view ever mounts, + // e.g. the very first time a brand-new conversation is opened in a fresh session -- which sets + // _maxSeq to that one prefetched message's seq without caching anything before it. From that + // point on withLaterData(MESSAGES_PAGE) asks for "later than the one message I already have" and + // gets back nothing: confirmed live, a brand-new 4-message P2P conversation opened this way + // rendered only the single latest message, silently missing the rest of the history. + // withData(undefined, undefined, limit) has no such since-cursor -- it always asks for "the + // latest `limit` messages" regardless of what the client thinks it already has, which is + // correct both in this poisoned-prefetch case and in the plain fresh-topic case (there + // topic._maxSeq is 0, so withLaterData(limit) already degrades to the exact same + // since:undefined query -- this is a strict fix, not a behavior change for the common path). + getQuery = getQuery.withData(undefined, undefined, MESSAGES_PAGE); if (this.state.isReader) { getQuery = getQuery.withLaterDel(); } @@ -685,7 +702,7 @@ class MessagesView extends React.Component { } const setQuery = newTopic ? this.props.newTopicParams : undefined; - topic.subscribe(getQuery.build(), setQuery) + const doSubscribe = () => topic.subscribe(getQuery.build(), setQuery) .then(ctrl => { if (ctrl.code == 303) { // Redirect to another topic requested. @@ -726,6 +743,24 @@ class MessagesView extends React.Component { blankState.title = this.props.intl.formatMessage(messages.not_found); this.setState(blankState); }); + + if (topic.isSubscribed()) { + // Same prefetch race as the comment on the data query above: the topic may already be + // "attached" server-side from prefetchLatestMessagePreviews' single-message subscribe, before + // this view ever asked for anything. The SDK's own Topic.subscribe() unconditionally + // short-circuits to `Promise.resolve(this)` once `_attached` is true (tinode.dev.js) + // *without even looking at the getParams passed in* -- calling topic.subscribe(getQuery...) + // directly in that state would silently no-op and this view would never receive the real + // history/pinned-messages/peer read-receipts it just asked for. leave(false) clears _attached + // (both client-side via _resetSub() and server-side, since it's a real round trip) without + // discarding already-cached messages (that only happens for leave(true), i.e. unsubscribe) -- + // once it resolves, the follow-up doSubscribe() is guaranteed to be a genuine, un-shortcut + // subscribe that gets the server to actually answer getQuery instead of silently dropping it. + topic.leave(false).catch(_ => { /* best-effort; doSubscribe() below still needs to run */ }).then(doSubscribe); + return; + } + + doSubscribe(); } leave(oldTopicName) {