DEV: Introduce history-store service (#24486)

This commit extracts the storage part of the route-scroll-manager into a dedicated service. This provides a key/value store which will reset for each navigation, and restore previous values when the user uses the back/forward buttons in their browser.

This gives us a reliable replacement for the old `DiscourseRoute.isPoppedState` function, which would not work under all situations.

Previously reverted in e6370decfd. This version has been significantly refactored, and includes an additional system spec for the issue we identified.

app/assets/javascripts/discourse/app/components/topic-entrance.js

@@ -33,6 +33,7 @@ function entranceDate(dt, showTime) {
 export default Component.extend(CleansUp, {
   router: service(),
   session: service(),
+  historyStore: service(),
   elementId: "topic-entrance",
   classNameBindings: ["visible::hidden"],
   topic: null,
@@ -166,10 +167,7 @@ export default Component.extend(CleansUp, {
   },
 
   _jumpTo(destination) {
-    this.session.set("lastTopicIdViewed", {
-      topicId: this.topic.id,
-      historyUuid: this.router.location.getState?.().uuid,
-    });
+    this.historyStore.set("lastTopicIdViewed", this.topic.id);
 
     this.cleanUp();
     DiscourseURL.routeTo(destination);

app/assets/javascripts/discourse/app/components/topic-list-item.js

@@ -37,14 +37,8 @@ export function showEntrance(e) {
 }
 
 export function navigateToTopic(topic, href) {
-  const owner = getOwner(this);
-  const router = owner.lookup("service:router");
-  const session = owner.lookup("service:session");
-
-  session.set("lastTopicIdViewed", {
-    topicId: topic.id,
-    historyUuid: router.location.getState?.().uuid,
-  });
+  const historyStore = getOwner(this).lookup("service:history-store");
+  historyStore.set("lastTopicIdViewed", topic.id);
 
   DiscourseURL.routeTo(href || topic.get("url"));
   return false;
@@ -52,6 +46,7 @@ export function navigateToTopic(topic, href) {
 
 export default Component.extend({
   router: service(),
+  historyStore: service(),
   tagName: "tr",
   classNameBindings: [":topic-list-item", "unboundClassNames", "topic.visited"],
   attributeBindings: ["data-topic-id", "role", "ariaLevel:aria-level"],
@@ -346,15 +341,11 @@ export default Component.extend({
 
   _highlightIfNeeded: on("didInsertElement", function () {
     // highlight the last topic viewed
-    const lastViewedTopicInfo = this.session.get("lastTopicIdViewed");
-
-    const isLastViewedTopic =
-      lastViewedTopicInfo?.topicId === this.topic.id &&
-      lastViewedTopicInfo?.historyUuid ===
-        this.router.location.getState?.().uuid;
+    const lastViewedTopicId = this.historyStore.get("lastTopicIdViewed");
+    const isLastViewedTopic = lastViewedTopicId === this.topic.id;
 
     if (isLastViewedTopic) {
-      this.session.set("lastTopicIdViewed", null);
+      this.historyStore.delete("lastTopicIdViewed");
       this.highlight({ isLastViewedTopic: true });
     } else if (this.get("topic.highlight")) {
       // highlight new topics that have been loaded from the server or the one we just created

app/assets/javascripts/discourse/app/routes/application.js

@@ -42,6 +42,7 @@ const ApplicationRoute = DiscourseRoute.extend({
   siteSettings: service(),
   clientErrorHandler: service(),
   login: service(),
+  historyStore: service(),
 
   get isOnlyOneExternalLoginMethod() {
     return (
@@ -63,6 +64,12 @@ const ApplicationRoute = DiscourseRoute.extend({
     return false;
   },
 
+  @action
+  willResolveModel(transition) {
+    this.historyStore.willResolveModel(transition);
+    return true;
+  },
+
   actions: {
     toggleMobileView() {
       mobile.toggleMobileView();

app/assets/javascripts/discourse/app/routes/build-category-route.js

@@ -21,6 +21,7 @@ class AbstractCategoryRoute extends DiscourseRoute {
   @service store;
   @service topicTrackingState;
   @service("search") searchService;
+  @service historyStore;
 
   queryParams = queryParams;
 
@@ -86,7 +87,7 @@ class AbstractCategoryRoute extends DiscourseRoute {
 
   async _retrieveTopicList(category, transition, modelParams) {
     const findOpts = filterQueryParams(modelParams, this.routeConfig);
-    const extras = { cached: this.isPoppedState(transition) };
+    const extras = { cached: this.historyStore.isPoppedState };
 
     let listFilter = `c/${Category.slugFor(category)}/${category.id}`;
     if (findOpts.no_subcategories) {

app/assets/javascripts/discourse/app/routes/build-topic-route.js

@@ -98,17 +98,18 @@ class AbstractTopicRoute extends DiscourseRoute {
   @service store;
   @service topicTrackingState;
   @service currentUser;
+  @service historyStore;
 
   queryParams = queryParams;
   templateName = "discovery/list";
   controllerName = "discovery/list";
 
-  async model(data, transition) {
+  async model(data) {
     // attempt to stop early cause we need this to be called before .sync
     this.screenTrack.stop();
 
     const findOpts = filterQueryParams(data),
-      findExtras = { cached: this.isPoppedState(transition) };
+      findExtras = { cached: this.historyStore.isPoppedState };
 
     const topicListPromise = findTopicList(
       this.store,

app/assets/javascripts/discourse/app/routes/discourse.js

@@ -65,13 +65,6 @@ const DiscourseRoute = Route.extend({
 
     return user.id === this.currentUser.id;
   },
-
-  isPoppedState(transition) {
-    return (
-      !transition._discourse_intercepted &&
-      (!!transition.intent.url || !!transition.queryParamsOnly)
-    );
-  },
 });
 
 export default DiscourseRoute;

app/assets/javascripts/discourse/app/routes/tag-show.js

@@ -26,6 +26,7 @@ export default class TagShowRoute extends DiscourseRoute {
   @service store;
   @service topicTrackingState;
   @service("search") searchService;
+  @service historyStore;
 
   queryParams = queryParams;
   controllerName = "discovery/list";
@@ -119,7 +120,7 @@ export default class TagShowRoute extends DiscourseRoute {
       filter,
       filteredQueryParams,
       {
-        cached: this.isPoppedState(transition),
+        cached: this.historyStore.isPoppedState,
       }
     );
 

app/assets/javascripts/discourse/app/routes/user-activity-bookmarks.js

@@ -1,4 +1,5 @@
 import { action } from "@ember/object";
+import { inject as service } from "@ember/service";
 import $ from "jquery";
 import { Promise } from "rsvp";
 import { ajax } from "discourse/lib/ajax";
@@ -6,6 +7,7 @@ import DiscourseRoute from "discourse/routes/discourse";
 import I18n from "discourse-i18n";
 
 export default DiscourseRoute.extend({
+  historyStore: service(),
   templateName: "user/bookmarks",
 
   queryParams: {
@@ -13,11 +15,11 @@ export default DiscourseRoute.extend({
     q: { refreshModel: true },
   },
 
-  model(params, transition) {
+  model(params) {
     const controller = this.controllerFor("user-activity-bookmarks");
 
     if (
-      this.isPoppedState(transition) &&
+      this.historyStore.isPoppedState &&
       this.session.bookmarksModel &&
       this.session.bookmarksModel.searchTerm === params.q
     ) {

app/assets/javascripts/discourse/app/services/history-store.js

@@ -0,0 +1,133 @@
+import { DEBUG } from "@glimmer/env";
+import Service, { inject as service } from "@ember/service";
+import { TrackedMap } from "@ember-compat/tracked-built-ins";
+import { disableImplicitInjections } from "discourse/lib/implicit-injections";
+import { isTesting } from "discourse-common/config/environment";
+import { bind } from "discourse-common/utils/decorators";
+
+const HISTORY_SIZE = 100;
+const HISTORIC_KEY = Symbol("historic");
+const HANDLED_TRANSITIONS = new WeakSet();
+
+/**
+ * This service provides a key-value store which can store per-route information.
+ * When navigating 'back' via browser controls, the service will restore the data
+ * for the appropriate route.
+ */
+@disableImplicitInjections
+export default class HistoryStore extends Service {
+  @service router;
+
+  #routeData = new Map();
+  #uuid;
+  #pendingStore;
+
+  get #currentStore() {
+    if (this.#pendingStore) {
+      return this.#pendingStore;
+    }
+
+    return this.#dataFor(this.#uuid);
+  }
+
+  /**
+   * Identify if the current route was accessed via the browser back/forward buttons
+   * @returns {boolean}
+   */
+  get isPoppedState() {
+    return !!this.get(HISTORIC_KEY);
+  }
+
+  /**
+   * Fetch a value from the current route's key/value store
+   */
+  get(key) {
+    return this.#currentStore.get(key);
+  }
+
+  /**
+   * Set a value in the current route's key/value store. Will persist for the lifetime
+   * of the route, and will be restored if the user navigates 'back' to the route.
+   */
+  set(key, value) {
+    return this.#currentStore.set(key, value);
+  }
+
+  /**
+   * Delete a value from the current route's key/value store
+   */
+  delete(key) {
+    return this.#currentStore.delete(key);
+  }
+
+  #pruneOldData() {
+    while (this.#routeData.size > HISTORY_SIZE) {
+      // JS Map guarantees keys will be returned in insertion order
+      const oldestUUID = this.#routeData.keys().next().value;
+      this.#routeData.delete(oldestUUID);
+    }
+  }
+
+  #dataFor(uuid) {
+    let data = this.#routeData.get(uuid);
+    if (data) {
+      return data;
+    }
+
+    data = new TrackedMap();
+    this.#routeData.set(uuid, data);
+    this.#pruneOldData();
+
+    return data;
+  }
+
+  /**
+   * Called by the Application route when its willResolveModel hook
+   * is triggered by the ember router. Unfortunately this hook is
+   * not available as an event on the router service.
+   */
+  @bind
+  willResolveModel(transition) {
+    if (HANDLED_TRANSITIONS.has(transition)) {
+      return;
+    }
+    HANDLED_TRANSITIONS.add(transition);
+
+    if (DEBUG && isTesting()) {
+      // Can't use window.history in tests
+      this.#pendingStore = new TrackedMap();
+      return;
+    }
+
+    this.set(HISTORIC_KEY, true);
+
+    let pendingStoreForThisTransition;
+
+    if (this.#uuid === window.history.state?.uuid) {
+      // A normal ember transition. The history uuid will only change **after** models are resolved.
+      // To allow routes to store data for the upcoming uuid, we set up a temporary data store
+      // and then persist it if/when the transition succeeds.
+      pendingStoreForThisTransition = new TrackedMap();
+    } else {
+      // A transition initiated by the browser back/forward buttons. We might already have some stored
+      // data for this route. If so, take a copy of it and use that as the pending store. As with normal transitions,
+      // it'll be persisted if/when the transition succeeds.
+      pendingStoreForThisTransition = new TrackedMap(
+        this.#dataFor(window.history.state?.uuid)?.entries()
+      );
+    }
+
+    this.#pendingStore = pendingStoreForThisTransition;
+    transition
+      .then(() => {
+        this.#uuid = window.history.state?.uuid;
+        this.#routeData.set(this.#uuid, this.#pendingStore);
+        this.#pruneOldData();
+      })
+      .finally(() => {
+        if (pendingStoreForThisTransition === this.#pendingStore) {
+          this.#pendingStore = null;
+        }
+      });
+  }
+}

app/assets/javascripts/discourse/app/services/route-scroll-manager.js

@@ -4,7 +4,7 @@ import { disableImplicitInjections } from "discourse/lib/implicit-injections";
 import { isTesting } from "discourse-common/config/environment";
 import { bind } from "discourse-common/utils/decorators";
 
-const MAX_SCROLL_LOCATIONS = 100;
+const STORE_KEY = Symbol("scroll-location");
 
 /**
  * This service is responsible for managing scroll position when transitioning.
@@ -18,9 +18,7 @@ const MAX_SCROLL_LOCATIONS = 100;
 @disableImplicitInjections
 export default class RouteScrollManager extends Service {
   @service router;
-
-  scrollLocationHistory = new Map();
-  uuid;
+  @service historyStore;
 
   scrollElement = isTesting()
     ? document.getElementById("ember-testing-container")
@@ -28,14 +26,10 @@ export default class RouteScrollManager extends Service {
 
   @bind
   routeWillChange() {
-    if (!this.uuid) {
-      return;
-    }
-    this.scrollLocationHistory.set(this.uuid, [
+    this.historyStore.set(STORE_KEY, [
       this.scrollElement.scrollLeft,
       this.scrollElement.scrollTop,
     ]);
-    this.#pruneOldScrollLocations();
   }
 
   @bind
@@ -44,34 +38,16 @@ export default class RouteScrollManager extends Service {
       return;
     }
 
-    const newUuid = this.router.location.getState?.().uuid;
-
-    if (newUuid === this.uuid) {
-      // routeDidChange fired without the history state actually changing. Most likely a refresh.
-      // Forget the previously-stored scroll location so that we scroll to the top
-      this.scrollLocationHistory.delete(this.uuid);
-    }
-
-    this.uuid = newUuid;
-
     if (!this.#shouldScroll(transition.to)) {
       return;
     }
 
-    const scrollLocation = this.scrollLocationHistory.get(this.uuid) || [0, 0];
+    const scrollLocation = this.historyStore.get(STORE_KEY) || [0, 0];
     schedule("afterRender", () => {
       this.scrollElement.scrollTo(...scrollLocation);
     });
   }
 
-  #pruneOldScrollLocations() {
-    while (this.scrollLocationHistory.size > MAX_SCROLL_LOCATIONS) {
-      // JS Map guarantees keys will be returned in insertion order
-      const oldestUUID = this.scrollLocationHistory.keys().next().value;
-      this.scrollLocationHistory.delete(oldestUUID);
-    }
-  }
-
   #shouldScroll(routeInfo) {
     // Leafmost route has priority
     for (let route = routeInfo; route; route = route.parent) {

spec/system/topic_list_focus_spec.rb

@@ -81,4 +81,26 @@ describe "Topic list focus", type: :system do
     expect(page).to have_css("body.navigation-topics")
     expect(focussed_topic_id).to eq(nil)
   end
+
+  it "refocusses properly when there are multiple pages of topics" do
+    extra_topics = Fabricate.times(25, :post).map(&:topic)
+    oldest_topic = Fabricate(:post).topic
+    oldest_topic.update(bumped_at: 1.day.ago)
+
+    visit("/latest")
+
+    # Scroll to bottom for infinite load
+    page.execute_script <<~JS
+      document.querySelectorAll('.topic-list-item')[24].scrollIntoView(true);
+    JS
+
+    # Click a topic
+    discovery.topic_list.visit_topic(oldest_topic)
+    expect(topic).to have_topic_title(oldest_topic.title)
+
+    # Going back to the topic-list should re-focus
+    page.go_back
+    expect(page).to have_css("body.navigation-topics")
+    expect(focussed_topic_id).to eq(oldest_topic.id)
+  end
 end