Initial tag load performance improvement (#87)

- Only load lastPostedDiscussion on TagsPage - For forum payload, only load top-level primary tags and top 3 secondary tags. - In other cases, load tags in dynamically when needed. Co-authored-by: Alexander Skvortsov <[email protected]> Co-authored-by: Sami Mazouz <[email protected]>
flarum · May 4, 2021 · cbdd7c8 · cbdd7c8
1 parent 2814d7f
commit cbdd7c8
Show file tree

Hide file tree

Showing 17 changed files with 257 additions and 47 deletions.
diff --git a/extend.php b/extend.php
@@ -45,6 +45,7 @@
         ->get('/tags', 'tags.index', Controller\ListTagsController::class)
         ->post('/tags', 'tags.create', Controller\CreateTagController::class)
         ->post('/tags/order', 'tags.order', Controller\OrderTagsController::class)
+        ->get('/tags/{slug}', 'tags.show', Controller\ShowTagController::class)
         ->patch('/tags/{id}', 'tags.update', Controller\UpdateTagController::class)
         ->delete('/tags/{id}', 'tags.delete', Controller\DeleteTagController::class),
 
@@ -80,7 +81,7 @@
         ->addInclude(['tags', 'tags.state']),
 
     (new Extend\ApiController(FlarumController\ShowForumController::class))
-        ->addInclude(['tags', 'tags.lastPostedDiscussion', 'tags.parent'])
+        ->addInclude(['tags', 'tags.parent'])
         ->prepareDataForSerialization(LoadForumTagsRelationship::class),
 
     (new Extend\Settings())

diff --git a/js/src/admin/components/TagsPage.js b/js/src/admin/components/TagsPage.js
@@ -2,6 +2,7 @@ import sortable from 'sortablejs';
 
 import ExtensionPage from 'flarum/components/ExtensionPage';
 import Button from 'flarum/components/Button';
+import LoadingIndicator from 'flarum/components/LoadingIndicator';
 import withAttr from 'flarum/utils/withAttr';
 
 import EditTagModal from './EditTagModal';
@@ -40,24 +41,38 @@ export default class TagsPage extends ExtensionPage {
     // we force a full reconstruction of the DOM by changing the key, which
     // makes mithril completely re-render the component on redraw.
     this.forcedRefreshKey = 0;
+
+    this.loading = true;
+
+    app.store.find('tags', { include: 'parent,lastPostedDiscussion' }).then(() => {
+      this.loading = false;
+
+      m.redraw();
+    });
   }
 
   content() {
+    if (this.loading) {
+      return <LoadingIndicator />;
+    }
+
     const minPrimaryTags = this.setting('flarum-tags.min_primary_tags', 0);
     const maxPrimaryTags = this.setting('flarum-tags.max_primary_tags', 0);
 
     const minSecondaryTags = this.setting('flarum-tags.min_secondary_tags', 0);
     const maxSecondaryTags = this.setting('flarum-tags.max_secondary_tags', 0);
 
+    const tags = sortTags(app.store.all('tags').filter(tag => !tag.parent()));
+
     return (
       <div className="TagsContent">
         <div className="TagsContent-list">
           <div className="container" key={this.forcedRefreshKey} oncreate={this.onListOnCreate.bind(this)}><div className="SettingsGroups">
             <div className="TagGroup">
               <label>{app.translator.trans('flarum-tags.admin.tags.primary_heading')}</label>
               <ol className="TagList TagList--primary">
-                {sortTags(app.store.all('tags'))
-                  .filter((tag) => tag.position() !== null && !tag.isChild())
+                {tags
+                  .filter(tag => tag.position() !== null && !tag.isChild())
                   .map(tagItem)}
               </ol>
               {Button.component(
@@ -73,9 +88,8 @@ export default class TagsPage extends ExtensionPage {
             <div className="TagGroup TagGroup--secondary">
               <label>{app.translator.trans('flarum-tags.admin.tags.secondary_heading')}</label>
               <ul className="TagList">
-                {app.store
-                  .all('tags')
-                  .filter((tag) => tag.position() === null)
+                {tags
+                  .filter(tag => tag.position() === null)
                   .sort((a, b) => a.name().localeCompare(b.name()))
                   .map(tagItem)}
               </ul>

diff --git a/js/src/common/models/Tag.js b/js/src/common/models/Tag.js
@@ -14,6 +14,7 @@ export default class Tag extends mixin(Model, {
 
   position: Model.attribute('position'),
   parent: Model.hasOne('parent'),
+  children: Model.hasMany('children'),
   defaultSort: Model.attribute('defaultSort'),
   isChild: Model.attribute('isChild'),
   isHidden: Model.attribute('isHidden'),

diff --git a/js/src/forum/addTagComposer.js b/js/src/forum/addTagComposer.js
@@ -9,7 +9,8 @@ import getSelectableTags from './utils/getSelectableTags';
 
 export default function () {
   extend(IndexPage.prototype, 'newDiscussionAction', function (promise) {
-    const tag = app.store.getBy('tags', 'slug', app.search.params().tags);
+    // From `addTagFilter
+    const tag = this.currentTag();
 
     if (tag) {
       const parent = tag.parent();
@@ -20,6 +21,11 @@ export default function () {
     }
   });
 
+
+  extend(DiscussionComposer.prototype, 'oninit', function () {
+    app.tagList.load(['parent']).then(() => m.redraw())
+  });
+
   // Add tag-selection abilities to the discussion composer.
   DiscussionComposer.prototype.chooseTags = function () {
     const selectableTags = getSelectableTags();

diff --git a/js/src/forum/addTagFilter.js b/js/src/forum/addTagFilter.js
@@ -7,9 +7,33 @@ import TagHero from './components/TagHero';
 
 export default function() {
   IndexPage.prototype.currentTag = function() {
+    if (this.currentActiveTag) {
+      return this.currentActiveTag;
+    }
+
     const slug = app.search.params().tags;
+    let tag = null;
+
+    if (slug) {
+      tag = app.store.getBy('tags', 'slug', slug);
+    }
+
+    if (slug && !tag || (tag && !tag.isChild() && !tag.children())) {
+      // Unlike the backend, no need to fetch parent.children because if we're on
+      // a child tag page, then either:
+      //    - We loaded in that child tag (and its siblings) in the API document
+      //    - We first navigated to the current tag's parent, which would have loaded in the current tag's siblings.
+      app.store.find('tags', slug, { include: 'children,children.parent,parent,state'}).then(() => {
+        this.currentActiveTag = app.store.getBy('tags', 'slug', slug);
 
-    if (slug) return app.store.getBy('tags', 'slug', slug);
+        m.redraw();
+      });
+    }
+
+    if (tag) {
+      this.currentActiveTag = tag;
+      return this.currentActiveTag;
+    }
   };
 
   // If currently viewing a tag, insert a tag hero at the top of the view.

diff --git a/js/src/forum/components/TagDiscussionModal.js b/js/src/forum/components/TagDiscussionModal.js
@@ -1,6 +1,7 @@
 import Modal from 'flarum/components/Modal';
 import DiscussionPage from 'flarum/components/DiscussionPage';
 import Button from 'flarum/components/Button';
+import LoadingIndicator from 'flarum/components/LoadingIndicator';
 import highlight from 'flarum/helpers/highlight';
 import classList from 'flarum/utils/classList';
 import extractText from 'flarum/utils/extractText';
@@ -16,21 +17,12 @@ export default class TagDiscussionModal extends Modal {
   oninit(vnode) {
     super.oninit(vnode);
 
-    this.tags = getSelectableTags(this.attrs.discussion);
-
-    this.tags = sortTags(this.tags);
+    this.tagsLoading = true;
 
     this.selected = [];
     this.filter = Stream('');
-    this.index = this.tags[0].id();
     this.focused = false;
 
-    if (this.attrs.selectedTags) {
-      this.attrs.selectedTags.map(this.addTag.bind(this));
-    } else if (this.attrs.discussion) {
-      this.attrs.discussion.tags().map(this.addTag.bind(this));
-    }
-
     this.minPrimary = app.forum.attribute('minPrimaryTags');
     this.maxPrimary = app.forum.attribute('maxPrimaryTags');
     this.minSecondary = app.forum.attribute('minSecondaryTags');
@@ -42,6 +34,22 @@ export default class TagDiscussionModal extends Modal {
       .onDown(() => this.setIndex(this.getCurrentNumericIndex() + 1, true))
       .onSelect(this.select.bind(this))
       .onRemove(() => this.selected.splice(this.selected.length - 1, 1));
+
+    app.tagList.load(['parent']).then(() => {
+      this.tagsLoading = false;
+
+      this.tags = sortTags(getSelectableTags(this.attrs.discussion));
+
+      if (this.attrs.selectedTags) {
+        this.attrs.selectedTags.map(this.addTag.bind(this));
+      } else if (this.attrs.discussion) {
+        this.attrs.discussion.tags().map(this.addTag.bind(this));
+      }
+
+      this.index = this.tags[0].id();
+
+      m.redraw();
+    });
   }
 
   primaryCount() {
@@ -113,6 +121,10 @@ export default class TagDiscussionModal extends Modal {
   }
 
   content() {
+    if (this.tagsLoading) {
+      return <LoadingIndicator />;
+    }
+
     let tags = this.tags;
     const filter = this.filter().toLowerCase();
     const primaryCount = this.primaryCount();

diff --git a/js/src/forum/components/TagsPage.js b/js/src/forum/components/TagsPage.js
@@ -1,6 +1,7 @@
 import Page from 'flarum/components/Page';
 import IndexPage from 'flarum/components/IndexPage';
 import Link from 'flarum/components/Link';
+import LoadingIndicator from 'flarum/components/LoadingIndicator';
 import listItems from 'flarum/helpers/listItems';
 import humanTime from 'flarum/helpers/humanTime';
 
@@ -12,12 +13,33 @@ export default class TagsPage extends Page {
   oninit(vnode) {
     super.oninit(vnode);
 
-    this.tags = sortTags(app.store.all('tags').filter(tag => !tag.parent()));
-
     app.history.push('tags', app.translator.trans('flarum-tags.forum.header.back_to_tags_tooltip'));
+
+    this.tags = [];
+
+    const preloaded = app.preloadedApiDocument();
+
+    if (preloaded) {
+      this.tags = sortTags(preloaded.filter(tag => !tag.isChild()));
+      return;
+    }    
+
+    this.loading = true;
+
+    app.tagList.load(['children', 'lastPostedDiscussion']).then(() => {
+      this.tags = sortTags(app.store.all('tags').filter(tag => !tag.isChild()));
+
+      this.loading = false;
+
+      m.redraw();
+    });
   }
 
   view() {
+    if (this.loading) {
+      return <LoadingIndicator />;
+    }
+
     const pinned = this.tags.filter(tag => tag.position() !== null);
     const cloud = this.tags.filter(tag => tag.position() === null);
 
@@ -33,7 +55,7 @@ export default class TagsPage extends Page {
             <ul className="TagTiles">
               {pinned.map(tag => {
                 const lastPostedDiscussion = tag.lastPostedDiscussion();
-                const children = sortTags(app.store.all('tags').filter(child => child.parent() === tag));
+                const children = sortTags(tag.children() || []);
 
                 return (
                   <li className={'TagTile ' + (tag.color() ? 'colored' : '')}

diff --git a/js/src/forum/index.js b/js/src/forum/index.js
@@ -6,6 +6,8 @@ import Tag from '../common/models/Tag';
 import TagsPage from './components/TagsPage';
 import DiscussionTaggedPost from './components/DiscussionTaggedPost';
 
+import TagListState from './states/TagListState';
+
 import addTagList from './addTagList';
 import addTagFilter from './addTagFilter';
 import addTagLabels from './addTagLabels';
@@ -22,6 +24,8 @@ app.initializers.add('flarum-tags', function(app) {
 
   app.store.models.tags = Tag;
 
+  app.tagList = new TagListState();
+
   Discussion.prototype.tags = Model.hasMany('tags');
   Discussion.prototype.canTag = Model.attribute('canTag');
 

diff --git a/js/src/forum/states/TagListState.js b/js/src/forum/states/TagListState.js
@@ -0,0 +1,20 @@
+export default class TagListState {
+    constructor() {
+        this.loadedIncludes = new Set();
+    }
+
+    async load(includes = []) {
+        const unloadedIncludes = includes.filter(include => !this.loadedIncludes.has(include));
+
+        if (unloadedIncludes.length === 0) {
+            return Promise.resolve(app.store.all('tags'));
+        }
+
+        return app.store
+            .find('tags', { include: 'parent,lastPostedDiscussion' })
+            .then(val => {
+                unloadedIncludes.forEach(include => this.loadedIncludes.add(include));
+                return val;
+            });
+    }
+}
diff --git a/src/Api/Controller/ListTagsController.php b/src/Api/Controller/ListTagsController.php
@@ -27,14 +27,16 @@ class ListTagsController extends AbstractListController
      * {@inheritdoc}
      */
     public $include = [
-        'parent',
+        'parent'
     ];
 
     /**
      * {@inheritdoc}
      */
     public $optionalInclude = [
+        'children',
         'lastPostedDiscussion',
+        'state'
     ];
 
     /**
@@ -58,6 +60,10 @@ protected function data(ServerRequestInterface $request, Document $document)
         $actor = RequestUtil::getActor($request);
         $include = $this->extractInclude($request);
 
+        if (in_array('lastPostedDiscussion', $include)) {
+            $include = array_merge($include, ['lastPostedDiscussion.tags', 'lastPostedDiscussion.state']);
+        }
+
         $tags = $this->tags->whereVisibleTo($actor)->withStateFor($actor)->get();
 
         return $tags->load($include);

diff --git a/src/Api/Controller/ShowTagController.php b/src/Api/Controller/ShowTagController.php
@@ -0,0 +1,58 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tags\Api\Controller;
+
+use Flarum\Api\Controller\AbstractShowController;
+use Flarum\Tags\Api\Serializer\TagSerializer;
+use Flarum\Tags\Tag;
+use Illuminate\Support\Arr;
+use Psr\Http\Message\ServerRequestInterface;
+use Tobscure\JsonApi\Document;
+
+class ShowTagController extends AbstractShowController
+{
+    public $serializer = TagSerializer::class;
+
+    public $optionalInclude = [
+        'children',
+        'children.parent',
+        'lastPostedDiscussion',
+        'parent',
+        'parent.children',
+        'parent.children.parent',
+        'state'
+    ];
+
+    /**
+     * @var Tag
+     */
+    private $tags;
+
+    public function __construct(Tag $tags)
+    {
+        $this->tags = $tags;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    protected function data(ServerRequestInterface $request, Document $document)
+    {
+        $slug = Arr::get($request->getQueryParams(), 'slug');
+        $actor = $request->getAttribute('actor');
+        $include = $this->extractInclude($request);
+
+        return $this->tags
+            ->whereVisibleTo($actor)
+            ->with($include)
+            ->where('slug', $slug)
+            ->firstOrFail();
+    }
+}