From 9610aea18949267b63277901e0b04bc944c76eab Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 10 Feb 2022 13:09:28 +1100 Subject: [PATCH] FEATURE: cache last post number (#15772) Instead of relaying on /timings request, we should cache last read post number. That should protect from having incorrect unread counter when going back to topic list. This additional cache is very temporary as once /timings request is finished, serializer will have a correct result. Simplified flow is: 1. Store in cache information about last seen post number before /timings request is sent 2. When getting back to topic list compare value of last seen post number returned by /latest request and information in cache. If cache number is higher, than use it instead of information returned by /latest. In addition delete cache item as there is high chance that `/timings` request already finished. 3. Optionally, delete cache when timings request is done and topic list was not yet visited. Keeping cache reasonably small should not affect performance. --- .../discourse/app/lib/topic-list-tracker.js | 30 +++++++++++++++++++ .../discourse/app/services/screen-track.js | 27 +++++++++++++++-- .../discourse/tests/helpers/qunit-helpers.js | 6 +++- .../tests/unit/services/screen-track-test.js | 11 +++++-- 4 files changed, 69 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/topic-list-tracker.js b/app/assets/javascripts/discourse/app/lib/topic-list-tracker.js index 5134788cb61..06f2888da10 100644 --- a/app/assets/javascripts/discourse/app/lib/topic-list-tracker.js +++ b/app/assets/javascripts/discourse/app/lib/topic-list-tracker.js @@ -1,8 +1,22 @@ import { Promise } from "rsvp"; let model, currentTopicId; +let lastTopicId, lastHighestRead; + export function setTopicList(incomingModel) { model = incomingModel; + + model?.topics?.forEach((topic) => { + let highestRead = getHighestReadCache(topic.id); + if (highestRead && highestRead >= topic.last_read_post_number) { + let count = Math.max(topic.highest_post_number - highestRead, 0); + topic.setProperties({ + unread_posts: count, + new_posts: count, + }); + resetHighestReadCache(); + } + }); currentTopicId = null; } @@ -14,6 +28,22 @@ export function previousTopicUrl() { return urlAt(-1); } +export function setHighestReadCache(topicId, postNumber) { + lastTopicId = topicId; + lastHighestRead = postNumber; +} + +export function getHighestReadCache(topicId) { + if (topicId === lastTopicId) { + return lastHighestRead; + } +} + +export function resetHighestReadCache() { + lastTopicId = undefined; + lastHighestRead = undefined; +} + function urlAt(delta) { if (!model || !model.topics) { return Promise.resolve(null); diff --git a/app/assets/javascripts/discourse/app/services/screen-track.js b/app/assets/javascripts/discourse/app/services/screen-track.js index 647c3995f22..9b2ae7f740f 100644 --- a/app/assets/javascripts/discourse/app/services/screen-track.js +++ b/app/assets/javascripts/discourse/app/services/screen-track.js @@ -2,6 +2,11 @@ import Service, { inject as service } from "@ember/service"; import { ajax } from "discourse/lib/ajax"; import { bind } from "discourse-common/utils/decorators"; import { isTesting } from "discourse-common/config/environment"; +import { + getHighestReadCache, + resetHighestReadCache, + setHighestReadCache, +} from "discourse/lib/topic-list-tracker"; // We use this class to track how long posts in a topic are on the screen. const PAUSE_UNLESS_SCROLLED = 1000 * 60 * 3; @@ -128,9 +133,19 @@ export default class ScreenTrack extends Service { this._consolidatedTimings.push({ timings, topicTime, topicId }); } + const highestRead = parseInt(Object.keys(timings).lastObject, 10); + const cachedHighestRead = this.highestReadFromCache(topicId); + if (!cachedHighestRead || cachedHighestRead < highestRead) { + setHighestReadCache(topicId, highestRead); + } + return this._consolidatedTimings; } + highestReadFromCache(topicId) { + return getHighestReadCache(topicId); + } + sendNextConsolidatedTiming() { if (this._consolidatedTimings.length === 0) { return; @@ -172,11 +187,19 @@ export default class ScreenTrack extends Service { if (topicController) { const postNumbers = Object.keys(timings).map((v) => parseInt(v, 10)); topicController.readPosts(topicId, postNumbers); + + const cachedHighestRead = this.highestReadFromCache(topicId); + if ( + cachedHighestRead && + cachedHighestRead <= postNumbers.lastObject + ) { + resetHighestReadCache(topicId); + } } this.appEvents.trigger("topic:timings-sent", data); }) .catch((e) => { - if (ALLOWED_AJAX_FAILURES.indexOf(e.jqXHR.status) > -1) { + if (e.jqXHR && ALLOWED_AJAX_FAILURES.indexOf(e.jqXHR.status) > -1) { const delay = AJAX_FAILURE_DELAYS[this._ajaxFailures]; this._ajaxFailures += 1; @@ -187,7 +210,7 @@ export default class ScreenTrack extends Service { } } - if (window.console && window.console.warn) { + if (window.console && window.console.warn && e.jqXHR) { window.console.warn( `Failed to update topic times for topic ${topicId} due to ${e.jqXHR.status} error` ); diff --git a/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js b/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js index 1440a393f94..6c54d607803 100644 --- a/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js +++ b/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js @@ -39,7 +39,10 @@ import { resetCardClickListenerSelector } from "discourse/mixins/card-contents-b import { resetComposerCustomizations } from "discourse/models/composer"; import { resetQuickSearchRandomTips } from "discourse/widgets/search-menu-results"; import sessionFixtures from "discourse/tests/fixtures/session-fixtures"; -import { setTopicList } from "discourse/lib/topic-list-tracker"; +import { + resetHighestReadCache, + setTopicList, +} from "discourse/lib/topic-list-tracker"; import sinon from "sinon"; import siteFixtures from "discourse/tests/fixtures/site-fixtures"; import { clearResolverOptions } from "discourse-common/resolver"; @@ -160,6 +163,7 @@ function testCleanup(container, app) { resetOneboxCache(); resetCustomPostMessageCallbacks(); resetUserSearchCache(); + resetHighestReadCache(); resetCardClickListenerSelector(); resetComposerCustomizations(); resetQuickSearchRandomTips(); diff --git a/app/assets/javascripts/discourse/tests/unit/services/screen-track-test.js b/app/assets/javascripts/discourse/tests/unit/services/screen-track-test.js index 7eeceb4b268..8a73f75066b 100644 --- a/app/assets/javascripts/discourse/tests/unit/services/screen-track-test.js +++ b/app/assets/javascripts/discourse/tests/unit/services/screen-track-test.js @@ -7,15 +7,22 @@ discourseModule("Unit | Service | screen-track", function () { tracker.consolidateTimings({ 1: 10, 2: 5 }, 10, 1); tracker.consolidateTimings({ 1: 5, 3: 1 }, 3, 1); - const consolidated = tracker.consolidateTimings({ 1: 5, 3: 1 }, 3, 2); + const consolidated = tracker.consolidateTimings({ 1: 5, 3: 1, 4: 5 }, 3, 2); assert.deepEqual( consolidated, [ { timings: { 1: 15, 2: 5, 3: 1 }, topicTime: 13, topicId: 1 }, - { timings: { 1: 5, 3: 1 }, topicTime: 3, topicId: 2 }, + { timings: { 1: 5, 3: 1, 4: 5 }, topicTime: 3, topicId: 2 }, ], "expecting consolidated timings to match correctly" ); + + tracker.sendNextConsolidatedTiming(); + assert.equal( + tracker.highestReadFromCache(2), + 4, + "caches highest read post number for second topic" + ); }); });