FIX: Always wait for promise when loading a topic (#10465)

FIX: Always wait for promise when loading a topic (#10465)

It turns out that setupController doesn’t always wait when returning a promise, but the model hook does. This fixes issues with the page:changed event firing before the transition has complete.

diff --git a/app/assets/javascripts/discourse/app/routes/topic-from-params.js b/app/assets/javascripts/discourse/app/routes/topic-from-params.js
index e2b7af2..5830a5e 100644
--- a/app/assets/javascripts/discourse/app/routes/topic-from-params.js
+++ b/app/assets/javascripts/discourse/app/routes/topic-from-params.js
@@ -9,93 +9,101 @@ import { isTesting } from "discourse-common/config/environment";
 export default DiscourseRoute.extend({
   // Avoid default model hook
   model(params) {
-    return params;
-  },
-
-  deactivate() {
-    this._super(...arguments);
-    this.controllerFor("topic").unsubscribe();
-  },
-
-  setupController(controller, params, { _discourse_anchor }) {
     params = params || {};
     params.track_visit = true;
 
-    const topic = this.modelFor("topic"),
-      postStream = topic.postStream,
-      topicController = this.controllerFor("topic"),
-      composerController = this.controllerFor("composer");
+    const topic = this.modelFor("topic");
+    const postStream = topic.postStream;
 
     // I sincerely hope no topic gets this many posts
     if (params.nearPost === "last") {
       params.nearPost = 999999999;
     }
-
     params.forceLoad = true;
 
     return postStream
       .refresh(params)
-      .then(() => {
-        // TODO we are seeing errors where closest post is null and this is exploding
-        // we need better handling and logging for this condition.
-
-        // there are no closestPost for hidden topics
-        if (topic.view_hidden) {
-          return;
-        }
-
-        // The post we requested might not exist. Let's find the closest post
-        const closestPost = postStream.closestPostForPostNumber(
-          params.nearPost || 1
-        );
-        const closest = closestPost.post_number;
-
-        topicController.setProperties({
-          "model.currentPost": closest,
-          enteredIndex: topic.postStream.progressIndexOfPost(closestPost),
-          enteredAt: Date.now().toString()
-        });
-
-        this.appEvents.trigger("page:topic-loaded", topic);
-        topicController.subscribe();
-
-        // Highlight our post after the next render
-        schedule("afterRender", () =>
-          this.appEvents.trigger("post:highlight", closest)
-        );
-
-        const opts = {};
-        if (document.location.hash) {
-          opts.anchor = document.location.hash.substr(1);
-        } else if (_discourse_anchor) {
-          opts.anchor = _discourse_anchor;
-        }
-        DiscourseURL.jumpToPost(closest, opts);
-
-        // completely clear out all the bookmark related attributes
-        // because they are not in the response if bookmarked == false
-        if (closestPost && !closestPost.bookmarked) {
-          closestPost.clearBookmark();
-        }
-
-        if (!isEmpty(topic.draft)) {
-          composerController.open({
-            draft: Draft.getLocal(topic.draft_key, topic.draft),
-            draftKey: topic.draft_key,
-            draftSequence: topic.draft_sequence,
-            ignoreIfChanged: true,
-            topic
-          });
-        }
-      })
+      .then(() => params)
       .catch(e => {
         if (!isTesting()) {
           // eslint-disable-next-line no-console
           console.log("Could not view topic", e);
         }
+        params._loading_error = true;
+        return params;
       });
   },
 
+  deactivate() {
+    this._super(...arguments);
+    this.controllerFor("topic").unsubscribe();
+  },
+
+  setupController(controller, params, { _discourse_anchor }) {
+    // Don't do anything else if we couldn't load
+    // TODO: Tests require this but it seems bad
+    if (params._loading_error) {
+      return;
+    }
+
+    const topicController = this.controllerFor("topic");
+    const composerController = this.controllerFor("composer");
+    const topic = this.modelFor("topic");
+    const postStream = topic.postStream;
+
+    // TODO we are seeing errors where closest post is null and this is exploding
+    // we need better handling and logging for this condition.
+
+    // there are no closestPost for hidden topics
+    if (topic.view_hidden) {
+      return;
+    }
+
+    // The post we requested might not exist. Let's find the closest post
+    const closestPost = postStream.closestPostForPostNumber(
+      params.nearPost || 1
+    );
+    const closest = closestPost.post_number;
+
+    topicController.setProperties({
+      "model.currentPost": closest,
+      enteredIndex: topic.postStream.progressIndexOfPost(closestPost),
+      enteredAt: Date.now().toString()
+    });
+
+    this.appEvents.trigger("page:topic-loaded", topic);
+    topicController.subscribe();
+
+    // Highlight our post after the next render
+    schedule("afterRender", () =>
+      this.appEvents.trigger("post:highlight", closest)
+    );
+
+    const opts = {};
+    if (document.location.hash) {
+      opts.anchor = document.location.hash.substr(1);
+    } else if (_discourse_anchor) {
+      opts.anchor = _discourse_anchor;
+    }
+    DiscourseURL.jumpToPost(closest, opts);
+
+    // completely clear out all the bookmark related attributes
+    // because they are not in the response if bookmarked == false
+    if (closestPost && !closestPost.bookmarked) {
+      closestPost.clearBookmark();
+    }
+
+    if (!isEmpty(topic.draft)) {
+      composerController.open({
+        draft: Draft.getLocal(topic.draft_key, topic.draft),
+        draftKey: topic.draft_key,
+        draftSequence: topic.draft_sequence,
+        ignoreIfChanged: true,
+        topic
+      });
+    }
+  },
+
   actions: {
     willTransition() {
       this.controllerFor("topic").set(

GitHub sha: bad7c287

1 Like

This commit appears in #10465 which was merged by eviltrout.