FIX: Simplify sidebar custom link implementation (#26201)

All our link validation, and conversion from url -> route/model/query is expensive and prone to bugs. Instead, if people enter a link, we can just use it as-is. Originally all this extra logic was added to handle unusual situations like `/safe-mode`, `/my/...`, etc. However, all of these are now handled correctly by our Ember router, so there is no need for it. Now, we just pass the user-supplied `href` directly to the SectionLink component, and let Ember handle routing to it when clicked. The only functional change here is that we no longer validate internal links by parsing them with the Ember router. But I'd argue this is fine, because the previous logic would cause both false positives (e.g. `/t/123` would be valid, even if topic 123 doesn't exist), and false negatives (for routes which are server-side only, like the new AI share pages).
discourse · Mar 20, 2024 · e3cfb19 · e3cfb19 · discoursebot · Mar 20, 2024
1 parent a884842
commit e3cfb19
Show file tree

Hide file tree

Showing 11 changed files with 103 additions and 199 deletions.
diff --git a/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.js b/app/assets/javascripts/discourse/app/components/modal/sidebar-section-form.js
@@ -7,17 +7,10 @@ import { isEmpty } from "@ember/utils";
 import { ajax } from "discourse/lib/ajax";
 import { extractError } from "discourse/lib/ajax-error";
 import { SIDEBAR_SECTION, SIDEBAR_URL } from "discourse/lib/constants";
-import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper";
 import { sanitize } from "discourse/lib/text";
 import { afterRender, bind } from "discourse-common/utils/decorators";
 import I18n from "discourse-i18n";
 
-const FULL_RELOAD_LINKS_REGEX = [
-  /^\/my\/[a-z_\-\/]+$/,
-  /^\/pub\/[a-z_\-\/]+$/,
-  /^\/safe-mode$/,
-];
-
 class Section {
   @tracked title;
   @tracked links;
@@ -213,28 +206,16 @@ class SectionLink {
   }
 
   get #invalidValue() {
-    return (
-      this.path &&
-      (this.external ? !this.#validExternal() : !this.#validInternal())
-    );
+    return this.path && !this.#validLink();
   }
 
-  #validExternal() {
+  #validLink() {
     try {
-      return new URL(this.value);
+      return new URL(this.value, document.location.origin);
     } catch {
       return false;
     }
   }
-
-  #validInternal() {
-    const routeInfoHelper = new RouteInfoHelper(this.router, this.path);
-
-    return (
-      routeInfoHelper.route !== "unknown" ||
-      FULL_RELOAD_LINKS_REGEX.some((regex) => this.path.match(regex))
-    );
-  }
 }
 
 export default class SidebarSectionForm extends Component {

diff --git a/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/common/custom-section.hbs
@@ -9,38 +9,25 @@
   class={{this.section.dragCss}}
 >
   {{#each this.section.links as |link|}}
-    {{#if link.externalOrFullReload}}
-      <Sidebar::SectionLink
-        @shouldDisplay={{link.shouldDisplay}}
-        @linkName={{link.name}}
-        @content={{replace-emoji link.text}}
-        @prefixType="icon"
-        @prefixValue={{link.prefixValue}}
-        @fullReload={{link.fullReload}}
-        @href={{link.value}}
-        @linkClass={{link.linkDragCss}}
-      />
-    {{else}}
-      <Sidebar::SectionLink
-        @shouldDisplay={{link.shouldDisplay}}
-        @href={{link.href}}
-        @title={{link.title}}
-        @linkName={{link.name}}
-        @route={{link.route}}
-        @model={{link.model}}
-        @models={{link.models}}
-        @query={{link.query}}
-        @content={{replace-emoji link.text}}
-        @badgeText={{link.badgeText}}
-        @prefixType="icon"
-        @prefixValue={{link.prefixValue}}
-        @suffixCSSClass={{link.suffixCSSClass}}
-        @suffixValue={{link.suffixValue}}
-        @suffixType={{link.suffixType}}
-        @currentWhen={{link.currentWhen}}
-        @linkClass={{link.linkDragCss}}
-      />
-    {{/if}}
+    <Sidebar::SectionLink
+      @badgeText={{link.badgeText}}
+      @content={{replace-emoji link.text}}
+      @currentWhen={{link.currentWhen}}
+      @href={{or link.value link.href}}
+      @linkClass={{link.linkDragCss}}
+      @linkName={{link.name}}
+      @model={{link.model}}
+      @models={{link.models}}
+      @prefixType="icon"
+      @prefixValue={{link.prefixValue}}
+      @query={{link.query}}
+      @route={{link.route}}
+      @shouldDisplay={{link.shouldDisplay}}
+      @suffixCSSClass={{link.suffixCSSClass}}
+      @suffixType={{link.suffixType}}
+      @suffixValue={{link.suffixValue}}
+      @title={{link.title}}
+    />
   {{/each}}
 
   {{#if this.section.moreLinks}}

diff --git a/app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs b/app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs
@@ -1,30 +1,18 @@
-{{#if @sectionLink.externalOrFullReload}}
-  <Sidebar::SectionLink
-    @shouldDisplay={{@sectionLink.shouldDisplay}}
-    @linkName={{@sectionLink.name}}
-    @content={{replace-emoji @sectionLink.text}}
-    @prefixType="icon"
-    @prefixValue={{@sectionLink.prefixValue}}
-    @fullReload={{@sectionLink.fullReload}}
-    @href={{@sectionLink.value}}
-  />
-{{else}}
-  <Sidebar::SectionLink
-    @shouldDisplay={{@sectionLink.shouldDisplay}}
-    @href={{@sectionLink.href}}
-    @title={{@sectionLink.title}}
-    @linkName={{@sectionLink.name}}
-    @route={{@sectionLink.route}}
-    @model={{@sectionLink.model}}
-    @models={{@sectionLink.models}}
-    @query={{@sectionLink.query}}
-    @content={{replace-emoji @sectionLink.text}}
-    @badgeText={{@sectionLink.badgeText}}
-    @prefixType="icon"
-    @prefixValue={{@sectionLink.prefixValue}}
-    @suffixCSSClass={{@sectionLink.suffixCSSClass}}
-    @suffixValue={{@sectionLink.suffixValue}}
-    @suffixType={{@sectionLink.suffixType}}
-    @currentWhen={{@sectionLink.currentWhen}}
-  />
-{{/if}}
+<Sidebar::SectionLink
+  @badgeText={{@sectionLink.badgeText}}
+  @content={{replace-emoji @sectionLink.text}}
+  @currentWhen={{@sectionLink.currentWhen}}
+  @href={{or @sectionLink.href @sectionLink.value}}
+  @linkName={{@sectionLink.name}}
+  @model={{@sectionLink.model}}
+  @models={{@sectionLink.models}}
+  @prefixType="icon"
+  @prefixValue={{@sectionLink.prefixValue}}
+  @query={{@sectionLink.query}}
+  @route={{@sectionLink.route}}
+  @shouldDisplay={{@sectionLink.shouldDisplay}}
+  @suffixCSSClass={{@sectionLink.suffixCSSClass}}
+  @suffixType={{@sectionLink.suffixType}}
+  @suffixValue={{@sectionLink.suffixValue}}
+  @title={{@sectionLink.title}}
+/>
diff --git a/app/assets/javascripts/discourse/app/components/sidebar/section-link.js b/app/assets/javascripts/discourse/app/components/sidebar/section-link.js
@@ -58,14 +58,20 @@ export default class SectionLink extends Component {
   }
 
   get target() {
-    if (this.args.fullReload) {
-      return "_self";
-    }
-    return this.currentUser?.user_option?.external_links_in_new_tab
+    return this.currentUser?.user_option?.external_links_in_new_tab &&
+      this.isExternal
       ? "_blank"
       : "_self";
   }
 
+  get isExternal() {
+    return (
+      this.args.href &&
+      new URL(this.args.href, window.location.href).origin !==
+        window.location.origin
+    );
+  }
+
   get models() {
     if (this.args.model) {
       return [this.args.model];

diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js
@@ -47,11 +47,9 @@ export default class BaseCommunitySectionLink {
   }
 
   /**
-   * @returns {string} Ember route
+   * @returns {string|undefined} Ember route
    */
-  get route() {
-    this._notImplemented();
-  }
+  get route() {}
 
   /**
    * @returns {string} href attribute for the link. This property will take precedence over the `route` property when set.

diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/custom-community-section-links.js b/app/assets/javascripts/discourse/app/lib/sidebar/custom-community-section-links.js
@@ -1,5 +1,4 @@
 import BaseCommunitySectionLink from "discourse/lib/sidebar/base-community-section-link";
-import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper";
 
 export let customSectionLinks = [];
 export let secondaryCustomSectionLinks = [];
@@ -27,38 +26,10 @@ export function addSectionLink(args, secondary) {
     links.push(args.call(this, BaseCommunitySectionLink));
   } else {
     const klass = class extends BaseCommunitySectionLink {
-      constructor() {
-        super(...arguments);
-
-        if (args.href) {
-          this.routeInfoHelper = new RouteInfoHelper(this.router, args.href);
-        }
-      }
-
       get name() {
         return args.name;
       }
 
-      get route() {
-        if (args.href) {
-          return this.routeInfoHelper.route;
-        } else {
-          return args.route;
-        }
-      }
-
-      get models() {
-        if (args.href) {
-          return this.routeInfoHelper.models;
-        }
-      }
-
-      get query() {
-        if (args.href) {
-          return this.routeInfoHelper.query;
-        }
-      }
-
       get text() {
         return args.text;
       }
@@ -67,6 +38,14 @@ export function addSectionLink(args, secondary) {
         return args.title;
       }
 
+      get href() {
+        return args.href;
+      }
+
+      get route() {
+        return args.route;
+      }
+
       get prefixValue() {
         return args.icon || super.prefixValue;
       }

diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/route-info-helper.js b/app/assets/javascripts/discourse/app/lib/sidebar/route-info-helper.js
diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js b/app/assets/javascripts/discourse/app/lib/sidebar/section-link.js
@@ -1,44 +1,20 @@
 import { tracked } from "@glimmer/tracking";
-import RouteInfoHelper from "discourse/lib/sidebar/route-info-helper";
-import { defaultHomepage } from "discourse/lib/utilities";
 
 export default class SectionLink {
   @tracked linkDragCss;
 
-  constructor(
-    { external, full_reload, icon, id, name, value },
-    section,
-    router
-  ) {
+  constructor({ external, icon, id, name, value }, section) {
     this.external = external;
-    this.fullReload = full_reload;
     this.prefixValue = icon;
     this.id = id;
     this.name = name;
     this.text = name;
     this.value = value;
     this.section = section;
     this.withAnchor = value.match(/#\w+$/gi);
-
-    if (!this.externalOrFullReload) {
-      const routeInfoHelper = new RouteInfoHelper(router, value);
-
-      if (routeInfoHelper.route === "discovery.index") {
-        this.route = `discovery.${defaultHomepage()}`;
-      } else {
-        this.route = routeInfoHelper.route;
-      }
-
-      this.models = routeInfoHelper.models;
-      this.query = routeInfoHelper.query;
-    }
   }
 
   get shouldDisplay() {
     return true;
   }
-
-  get externalOrFullReload() {
-    return this.external || this.fullReload || this.withAnchor;
-  }
 }
diff --git a/app/models/sidebar_url.rb b/app/models/sidebar_url.rb
@@ -3,7 +3,6 @@
 class SidebarUrl < ActiveRecord::Base
   enum :segment, { primary: 0, secondary: 1 }, scopes: false, suffix: true
 
-  FULL_RELOAD_LINKS_REGEX = [%r{\A/my/[a-z_\-/]+\z}, %r{\A/pub/[a-z_\-/]+\z}, %r{\A/safe-mode\z}]
   MAX_ICON_LENGTH = 40
   MAX_NAME_LENGTH = 80
   MAX_VALUE_LENGTH = 1000
@@ -69,10 +68,6 @@ def remove_internal_hostname
   def set_external
     self.external = value.start_with?("http://", "https://")
   end
-
-  def full_reload?
-    FULL_RELOAD_LINKS_REGEX.any? { |regex| value =~ regex }
-  end
 end
 
 # == Schema Information

diff --git a/app/serializers/sidebar_url_serializer.rb b/app/serializers/sidebar_url_serializer.rb
@@ -1,13 +1,9 @@
 # frozen_string_literal: true
 
 class SidebarUrlSerializer < ApplicationSerializer
-  attributes :id, :name, :value, :icon, :external, :full_reload, :segment
+  attributes :id, :name, :value, :icon, :external, :segment
 
   def external
     object.external?
   end
-
-  def full_reload
-    object.full_reload?
-  end
 end