Revert history-store changes (#24471)

This reverts commit 20e562bd99, 161256eef8 and a8292d25f8.

It looks like this affected cache-restoration of topic lists in some circumstances. It also looks like routing behavior may vary when toggling the loading indicator between spinner and slider.

More investigation and testing required.

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

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

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

@@ -38,7 +38,8 @@ export function showEntrance(e) {
 
 export function navigateToTopic(topic, href) {
   const owner = getOwner(this);
-  const historyStore = owner.lookup("service:history-store");
+  const router = owner.lookup("service:router");
+  const session = owner.lookup("service:session");
   const siteSettings = owner.lookup("service:site-settings");
   const appEvents = owner.lookup("service:appEvents");
 
@@ -48,7 +49,10 @@ export function navigateToTopic(topic, href) {
     appEvents.trigger("header:update-topic", topic);
   }
 
-  historyStore.set("lastTopicIdViewed", topic.id);
+  session.set("lastTopicIdViewed", {
+    topicId: topic.id,
+    historyUuid: router.location.getState?.().uuid,
+  });
 
   DiscourseURL.routeTo(href || topic.get("url"));
   return false;
@@ -56,7 +60,6 @@ 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"],
@@ -351,11 +354,15 @@ export default Component.extend({
 
   _highlightIfNeeded: on("didInsertElement", function () {
     // highlight the last topic viewed
-    const lastViewedTopicId = this.historyStore.get("lastTopicIdViewed");
-    const isLastViewedTopic = lastViewedTopicId === this.topic.id;
+    const lastViewedTopicInfo = this.session.get("lastTopicIdViewed");
+
+    const isLastViewedTopic =
+      lastViewedTopicInfo?.topicId === this.topic.id &&
+      lastViewedTopicInfo?.historyUuid ===
+        this.router.location.getState?.().uuid;
 
     if (isLastViewedTopic) {
-      this.historyStore.delete("lastTopicIdViewed");
+      this.session.set("lastTopicIdViewed", null);
       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/build-category-route.js

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

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

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

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

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

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

@@ -1,5 +1,4 @@
 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";
@@ -7,7 +6,6 @@ import DiscourseRoute from "discourse/routes/discourse";
 import I18n from "discourse-i18n";
 
 export default DiscourseRoute.extend({
-  historyStore: service(),
   templateName: "user/bookmarks",
 
   queryParams: {
@@ -15,11 +13,11 @@ export default DiscourseRoute.extend({
     q: { refreshModel: true },
   },
 
-  model(params) {
+  model(params, transition) {
     const controller = this.controllerFor("user-activity-bookmarks");
 
     if (
-      this.historyStore.isPoppedState &&
+      this.isPoppedState(transition) &&
       this.session.bookmarksModel &&
       this.session.bookmarksModel.searchTerm === params.q
     ) {

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

@@ -1,92 +0,0 @@
-import Service, { inject as service } from "@ember/service";
-import { TrackedMap } from "@ember-compat/tracked-built-ins";
-import { disableImplicitInjections } from "discourse/lib/implicit-injections";
-import { bind } from "discourse-common/utils/decorators";
-
-const HISTORY_SIZE = 100;
-const HISTORIC_KEY = Symbol("historic");
-
-/**
- * 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;
-  #route;
-
-  constructor() {
-    super(...arguments);
-    this.router.on("routeDidChange", this.maybeRouteDidChange);
-  }
-
-  get #data() {
-    // Check if route changed since we last checked the uuid.
-    // This can happen if some other logic has a routeDidChange
-    // handler that runs before ours.
-    this.maybeRouteDidChange();
-
-    const uuid = this.#uuid;
-
-    let data = this.#routeData.get(uuid);
-    if (data) {
-      return data;
-    }
-
-    data = new TrackedMap();
-    this.#routeData.set(uuid, data);
-    this.#pruneOldData();
-
-    return data;
-  }
-
-  get isPoppedState() {
-    return !!this.get(HISTORIC_KEY);
-  }
-
-  get(key) {
-    return this.#data.get(key);
-  }
-
-  set(key, value) {
-    return this.#data.set(key, value);
-  }
-
-  delete(key) {
-    return this.#data.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);
-    }
-  }
-
-  @bind
-  maybeRouteDidChange() {
-    if (this.#route === this.router.currentRoute) {
-      return;
-    }
-    this.#route = this.router.currentRoute;
-    this.#routeData.get(this.#uuid)?.set(HISTORIC_KEY, true);
-
-    const newUuid = window.history.state?.uuid;
-
-    if (this.#uuid === newUuid) {
-      // A refresh. Clear the state
-      this.#routeData.delete(newUuid);
-    }
-
-    this.#uuid = newUuid;
-  }
-
-  willDestroy() {
-    this.router.off("routeDidChange", this.maybeRouteDidChange);
-  }
-}

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 STORE_KEY = Symbol("scroll-location");
+const MAX_SCROLL_LOCATIONS = 100;
 
 /**
  * This service is responsible for managing scroll position when transitioning.
@@ -18,7 +18,9 @@ const STORE_KEY = Symbol("scroll-location");
 @disableImplicitInjections
 export default class RouteScrollManager extends Service {
   @service router;
-  @service historyStore;
+
+  scrollLocationHistory = new Map();
+  uuid;
 
   scrollElement = isTesting()
     ? document.getElementById("ember-testing-container")
@@ -26,10 +28,14 @@ export default class RouteScrollManager extends Service {
 
   @bind
   routeWillChange() {
-    this.historyStore.set(STORE_KEY, [
+    if (!this.uuid) {
+      return;
+    }
+    this.scrollLocationHistory.set(this.uuid, [
       this.scrollElement.scrollLeft,
       this.scrollElement.scrollTop,
     ]);
+    this.#pruneOldScrollLocations();
   }
 
   @bind
@@ -38,16 +44,34 @@ 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.historyStore.get(STORE_KEY) || [0, 0];
+    const scrollLocation = this.scrollLocationHistory.get(this.uuid) || [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) {