Merge pull request #1028 from vector-im/bwindels/fix-1009

Fix backfill failing due to synapse pagination token format change

src/domain/session/room/timeline/tiles/GapTile.js

@@ -24,44 +24,37 @@ export class GapTile extends SimpleTile {
     constructor(entry, options) {
         super(entry, options);
         this._loading = false;
-        this._error = null;
+        this._waitingForConnection = false;
         this._isAtTop = true;
         this._siblingChanged = false;
-        this._showSpinner = false;
     }
 
     get needsDateSeparator() {
         return false;
     }
 
-    async fill() {
+    async fill(isRetrying = false) {
         if (!this._loading && !this._entry.edgeReached) {
             this._loading = true;
-            this._error = null;
-            this._showSpinner = true;
             this.emitChange("isLoading");
             try {
                 await this._room.fillGap(this._entry, 10);
             } catch (err) {
-                console.error(`room.fillGap(): ${err.message}:\n${err.stack}`);
-                this._error = err;
                 if (err instanceof ConnectionError) {
-                    this.emitChange("error");
-                    /*
-                    We need to wait for reconnection here rather than in
-                    notifyVisible() because when we return/throw here
-                    this._loading is set to false and other queued invocations of
-                    this method will succeed and attempt further room.fillGap() calls - 
-                    resulting in multiple error entries in logs and elsewhere!
-                    */
                     await this._waitForReconnection();
+                    if (!isRetrying) {
+                        // retry after the connection comes back
+                        // if this wasn't already a retry after coming back online
+                        return await this.fill(true);
+                    } else {
+                        return false;
+                    }
+                } else {
+                    this.reportError(err);
+                    return false;
                 }
-                // rethrow so caller of this method
-                // knows not to keep calling this for now
-                throw err;
             } finally {
                 this._loading = false;
-                this._showSpinner = false;
                 this.emitChange("isLoading");
             }
             return true;
@@ -70,25 +63,20 @@ export class GapTile extends SimpleTile {
     }
 
     async notifyVisible() {
+        // if any error happened before (apart from being offline),
+        // let the user dismiss the error before trying to backfill
+        // again so we don't try to do backfill the don't succeed
+        // in quick succession
+        if (this.errorViewModel) {
+            return;
+        }
         // we do (up to 10) backfills while no new tiles have been added to the timeline
         // because notifyVisible won't be called again until something gets added to the timeline
         let depth = 0;
         let canFillMore;
         this._siblingChanged = false;
         do {
-            try {
             canFillMore = await this.fill();
-            }
-            catch (e) {
-                if (e instanceof ConnectionError) {
-                    canFillMore = true;
-                    // Don't increase depth because this gap fill was a noop
-                    continue;
-                }
-                else {
-                    canFillMore = false;
-                }
-            }
             depth = depth + 1;
         } while (depth < 10 && !this._siblingChanged && canFillMore && !this.isDisposed);
     }
@@ -124,7 +112,11 @@ export class GapTile extends SimpleTile {
     }
 
     async _waitForReconnection() {
+        this._waitingForConnection = true;
+        this.emitUpdate("status");
         await this.options.client.reconnector.connectionStatus.waitFor(status => status === ConnectionStatus.Online).promise;
+        this._waitingForConnection = false;
+        this.emitUpdate("status");
     }
 
     get shape() {
@@ -136,29 +128,19 @@ export class GapTile extends SimpleTile {
     }
 
     get showSpinner() {
-        return this._showSpinner;
+        return this.isLoading || this._waitingForConnection;
     }
 
-    get error() {
+    get status() {
-        if (this._error) {
-            if (this._error instanceof ConnectionError) {
-                return "Waiting for reconnection";
-            }
         const dir = this._entry.prev_batch ? "previous" : "next";
-            return `Could not load ${dir} messages: ${this._error.message}`;
+        if (this._waitingForConnection) {
-        }
+            return "Waiting for connection…";
-        return null;
+        } else if (this.errorViewModel) {
-    }
+            return `Could not load ${dir} messages`;
-
+        } else if (this.isLoading) {
-    get currentAction() {
+            return "Loading more messages…";
-        if (this.error) {
+        } else {
-            return this.error;
+            return "Gave up loading more messages";
-        }
-        else if (this.isLoading) {
-            return "Loading";
-        }
-        else {
-            return "Not Loading";
         }
     }
 }

src/matrix/room/BaseRoom.js

@@ -350,7 +350,7 @@ export class BaseRoom extends EventEmitter {
                     fragmentIdComparer: this._fragmentIdComparer,
                     relationWriter
                 });
-                gapResult = await gapWriter.writeFragmentFill(fragmentEntry, response, txn, log);
+                gapResult = await gapWriter.writeFragmentFill(fragmentEntry, response, fragmentEntry.token, txn, log);
             } catch (err) {
                 txn.abort();
                 throw err;

src/matrix/room/timeline/persistence/GapWriter.js

@@ -154,10 +154,13 @@ export class GapWriter {
         return changedFragments;
     }
 
-    async writeFragmentFill(fragmentEntry, response, txn, log) {
+    /**
+     * @param {string} fromToken the token used to call /messages, to ensure it hasn't changed in storage 
+     */
+    async writeFragmentFill(fragmentEntry, response, fromToken, txn, log) {
         const {fragmentId, direction} = fragmentEntry;
         // chunk is in reverse-chronological order when backwards
-        const {chunk, start, state} = response;
+        const {chunk, state} = response;
         let {end} = response;
 
         if (!Array.isArray(chunk)) {
@@ -174,8 +177,8 @@ export class GapWriter {
         }
         fragmentEntry = fragmentEntry.withUpdatedFragment(fragment);
         // check that the request was done with the token we are aware of (extra care to avoid timeline corruption)
-        if (fragmentEntry.token !== start) {
+        if (fragmentEntry.token !== fromToken) {
-            throw new Error("start is not equal to prev_batch or next_batch");
+            throw new Error("The pagination token has changed locally while fetching messages.");
         }
 
         // begin (or end) of timeline reached
@@ -263,7 +266,7 @@ export function tests() {
     async function backfillAndWrite(mocks, fragmentEntry, limit) {
         const {txn, timelineMock, gapWriter} = mocks;
         const messageResponse = timelineMock.messages(fragmentEntry.token, undefined, fragmentEntry.direction.asApiString(), limit);
-        await gapWriter.writeFragmentFill(fragmentEntry, messageResponse, txn, logger);
+        await gapWriter.writeFragmentFill(fragmentEntry, messageResponse, fragmentEntry.token, txn, logger);
     }
 
     async function allFragmentEvents(mocks, fragmentId) {

src/platform/web/ui/css/themes/element/error.css

@@ -4,9 +4,14 @@
     margin: 16px;
 }
 
-.ErrorView_inline {
+.ErrorView.ErrorView_inline {
     color: var(--error-color);
-    margin: 4px;
+    margin: 4px 0;
+    padding: 4px 0;
+}
+
+.ErrorView.ErrorView_inline > p {
+    margin: 0;
 }
 
 .ErrorView {

src/platform/web/ui/css/themes/element/theme.css

@@ -769,6 +769,11 @@ a {
   margin: 0;
 }
 
+
+.FeatureView p {
+  margin: 8px 0;
+}
+
 .error {
     color: var(--error-color);
     font-weight: 600;

src/platform/web/ui/css/themes/element/timeline.css

@@ -411,7 +411,7 @@ only loads when the top comes into view*/
     border-radius: 10px;
 }
 
-.GapView > :not(:first-child) {
+.GapView_container > :not(:first-child) {
     margin-left: 12px;
 }
 

src/platform/web/ui/css/timeline.css

@@ -52,11 +52,11 @@ limitations under the License.
     align-items: center;
 }
 
-.GapView {
+.GapView_container {
     display: flex;
 }
 
-.GapView > :nth-child(2) {
+.GapView_container > span {
     flex: 1;
 }
 

src/platform/web/ui/session/room/timeline/GapView.js

@@ -16,6 +16,7 @@ limitations under the License.
 
 import {TemplateView} from "../../../general/TemplateView";
 import {spinner} from "../../../common.js";
+import {ErrorView} from "../../../general/ErrorView";
 
 export class GapView extends TemplateView {
     // ignore other argument
@@ -23,15 +24,20 @@ export class GapView extends TemplateView {
         super(vm);
     }
 
-    render(t) {
+    render(t, vm) {
         const className = {
             GapView: true,
             isLoading: vm => vm.isLoading,
             isAtTop: vm => vm.isAtTop,
         };
         return t.li({ className }, [
+            t.div({class: "GapView_container"}, [
                 t.if(vm => vm.showSpinner, (t) => spinner(t)),
-            t.span(vm => vm.currentAction)
+                t.span(vm => vm.status),
+            ]),
+            t.if(vm => !!vm.errorViewModel, t => {
+                return t.view(new ErrorView(vm.errorViewModel, {inline: true}));
+            })
         ]);
     }