DEV: Experimentally prefetch topic items using Foresight #33149
Conversation
This required a small refactor to the `Enter` and `o` shortcuts. A small delay was added to the click event, without it the navigation to the topic would often reset the `nearPost` parameter. Too fast, I guess.
5475e93 to
2442662
Compare
| } | ||
|
|
||
| e.preventDefault(); | ||
| discourseLater(() => selection.click(), 25); |
There was a problem hiding this comment.
The refactor in this file is due to a bug with the click event handler. When it got triggered without a delay, it would regularly cause the topic to go to the wrong post in the post stream. This is likely because with a cached JSON response, the transition happened too fast and our current post-stream code tries to correct too eagerly.
The 25ms delay seems to solve the issue. It's nice to have the Enter and o be split out of the generic _bindToClick handler.
There was a problem hiding this comment.
Adding an arbitrary delay feels like a workaround rather than a true fix. When we add these kind of delays, they tend to cause issues with other logic in the application.
I think we should try to isolate & fix the actual bug.
There was a problem hiding this comment.
Yes, true. I wonder if the glimmered post stream has the same issue. Will check.
| @action | ||
| didInsertTopicList() { | ||
| this.prefetch.clearPrefetchedTopics(); | ||
| } | ||
|
|
||
| @action | ||
| willDestroyTopicList() { | ||
| this.prefetch.clearPrefetchedTopics(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Modifiers should only be used for DOM-related things. Since this doesn't relate to the DOM, I think constructor/willDestroy would be a better fit?
| @action | ||
| triggerPrefetch() { | ||
| this.prefetch.register( | ||
| this.args.topic.id, | ||
| this.args.topic.last_read_post_number | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ideally this would be made as a proper modifier. @ember/render-modifiers are a compatibility thing which are not recommended for general use.
Also if it was a proper modifier, you could pass the element reference straight through to the service (so it wouldn't have to do a potentially-mismatching querySelector lookup). It could also have associated cleanup logic, so that the cache can be cleared when the link is removed from the DOM. That would avoid the need for the catch-all clearPrefetchedTopics() function
There was a problem hiding this comment.
Can you point to an example of a "proper" modifier?
| if (article.classList.contains("topic-list-item")) { | ||
| this.prefetchService.fetch( | ||
| article.dataset.topicId, | ||
| article.dataset.lastReadPostNumber | ||
| ); | ||
| } |
There was a problem hiding this comment.
Seems a bit weird to have this mixed up in keyboard shortcuts. How about making it happen during the onfocus event of a topic link?
There was a problem hiding this comment.
J/K doesn't set focus to the link, it sets it to the parent. I guess we could tie this to onfocus event on the article element. That would work. And set up a clear onblur.
| clearTopicsPastLimit() { | ||
| let topic_keys = []; | ||
| for (let key of this.data.keys()) { | ||
| if (key.startsWith("topic_")) { | ||
| topic_keys.push(key); | ||
| } | ||
| } | ||
|
|
||
| if (topic_keys.length > this.MAX_TOPIC_ENTRIES) { | ||
| for (let key of topic_keys) { | ||
| this.data.delete(key); | ||
| } | ||
| } | ||
| }, | ||
|
|
There was a problem hiding this comment.
So... when it hits 50... we're destroying the entire cache?
I would've expected something like a first-in-first-out strategy?
| context "when experimental_prefetch_allowed_groups is disabled" do | ||
| before { SiteSetting.experimental_prefetch_allowed_groups = "" } | ||
| include_examples "navigate to topic" | ||
| end |
There was a problem hiding this comment.
I think we need specs for the case where a topic is pre-fetched, and then things like posts/title/likes change before the user navigates to it. If we don't handle that case perfectly, we're gonna end up with a lot of weird and hard-to-debug issues.
This PR adds an experimental feature that prefetches topic JSON from topic lists. It uses the Foresight JS library to predict the user's mouse movements and trigger preloading the topic when the user's intention is to click on it. In preliminary testing, this results in considerably faster topic loading.
Prefetching has also been added to J/K keyboard navigation.
The PR also adds a mechanism for setting a limit to the preloaded topic cache, setting a maximum of 50 topics in the
PreloadStore. Once that threshold is hit, thetopic_IDcache is cleared.The feature is not ready for production, it is disabled by default and gated behind the
experimental_prefetch_allowed_groupssite setting.Testing this extensively is not possible AFAICT, our current test suite doesn't use mouse movements and even if it did, it would be very hard to reliably target elements.