FIX: Ensure topic route does not replaceState after navigation (#24563)

This hook is `cancel()`'d in a willTransition hook, but that isn't always enough. It might still be scheduled if there is a scroll event between `willTransition`, and the transition actually completing. Following c2d94be06e, this kind of scroll event happens when the loading indicator is set to 'spinner'. This would put the router in a weird state and cause navigation issues.

Also takes the opportunity to remove JQuery from this code path

https://meta.discourse.org/t/286463/15
This commit is contained in:
David Taylor 2023-11-27 11:35:59 +00:00 committed by GitHub
parent b1e43425bf
commit 86f3e86596
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -2,7 +2,6 @@ import { action, get } from "@ember/object";
import { cancel, schedule } from "@ember/runloop";
import { inject as service } from "@ember/service";
import { isEmpty } from "@ember/utils";
import $ from "jquery";
import AddPmParticipants from "discourse/components/modal/add-pm-participants";
import ChangeOwnerModal from "discourse/components/modal/change-owner";
import ChangeTimestampModal from "discourse/components/modal/change-timestamp";
@ -29,6 +28,7 @@ const TopicRoute = DiscourseRoute.extend({
composer: service(),
screenTrack: service(),
modal: service(),
router: service(),
scheduledReplace: null,
lastScrollPos: null,
@ -244,8 +244,7 @@ const TopicRoute = DiscourseRoute.extend({
// Use replaceState to update the URL once it changes
@action
postChangedRoute(currentPost) {
// do nothing if we are transitioning to another route
if (this.isTransitioning || TopicRoute.disableReplaceState) {
if (TopicRoute.disableReplaceState) {
return;
}
@ -279,11 +278,12 @@ const TopicRoute = DiscourseRoute.extend({
cancel(this.scheduledReplace);
this.setProperties({
lastScrollPos: parseInt($(document).scrollTop(), 10),
lastScrollPos: document.scrollingElement.scrollTop,
scheduledReplace: discourseLater(
this,
"_replaceUnlessScrolling",
postUrl,
topic.id,
SCROLL_DELAY
),
});
@ -299,18 +299,28 @@ const TopicRoute = DiscourseRoute.extend({
},
@action
willTransition(transition) {
willTransition() {
this._super(...arguments);
cancel(this.scheduledReplace);
this.set("isTransitioning", true);
transition.catch(() => this.set("isTransitioning", false));
return true;
},
// replaceState can be very slow on Android Chrome. This function debounces replaceState
// within a topic until scrolling stops
_replaceUnlessScrolling(url) {
const currentPos = parseInt($(document).scrollTop(), 10);
_replaceUnlessScrolling(url, topicId) {
const { currentRouteName } = this.router;
const stillOnTopicRoute = currentRouteName.split(".")[0] === "topic";
if (!stillOnTopicRoute) {
return;
}
const stillOnSameTopic = this.modelFor("topic").id === topicId;
if (!stillOnSameTopic) {
return;
}
const currentPos = document.scrollingElement.scrollTop;
if (currentPos === this.lastScrollPos) {
DiscourseURL.replaceState(url);
return;
@ -322,6 +332,7 @@ const TopicRoute = DiscourseRoute.extend({
this,
"_replaceUnlessScrolling",
url,
topicId,
SCROLL_DELAY
),
});
@ -368,11 +379,6 @@ const TopicRoute = DiscourseRoute.extend({
}
},
activate() {
this._super(...arguments);
this.set("isTransitioning", false);
},
deactivate() {
this._super(...arguments);
@ -393,9 +399,6 @@ const TopicRoute = DiscourseRoute.extend({
},
setupController(controller, model) {
// In case we navigate from one topic directly to another
this.set("isTransitioning", false);
controller.setProperties({
model,
editingTopic: false,