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).

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 (
+    return this.path && !this.#validLink();
-      this.path &&
-      (this.external ? !this.#validExternal() : !this.#validInternal())
-    );
   }
 
-  #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 {

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
-      <Sidebar::SectionLink
+      @badgeText={{link.badgeText}}
-        @shouldDisplay={{link.shouldDisplay}}
+      @content={{replace-emoji link.text}}
-        @linkName={{link.name}}
+      @currentWhen={{link.currentWhen}}
-        @content={{replace-emoji link.text}}
+      @href={{or link.value link.href}}
-        @prefixType="icon"
+      @linkClass={{link.linkDragCss}}
-        @prefixValue={{link.prefixValue}}
+      @linkName={{link.name}}
-        @fullReload={{link.fullReload}}
+      @model={{link.model}}
-        @href={{link.value}}
+      @models={{link.models}}
-        @linkClass={{link.linkDragCss}}
+      @prefixType="icon"
-      />
+      @prefixValue={{link.prefixValue}}
-    {{else}}
+      @query={{link.query}}
-      <Sidebar::SectionLink
+      @route={{link.route}}
-        @shouldDisplay={{link.shouldDisplay}}
+      @shouldDisplay={{link.shouldDisplay}}
-        @href={{link.href}}
+      @suffixCSSClass={{link.suffixCSSClass}}
-        @title={{link.title}}
+      @suffixType={{link.suffixType}}
-        @linkName={{link.name}}
+      @suffixValue={{link.suffixValue}}
-        @route={{link.route}}
+      @title={{link.title}}
-        @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}}
   {{/each}}
 
   {{#if this.section.moreLinks}}

app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs

@@ -1,30 +1,18 @@
-{{#if @sectionLink.externalOrFullReload}}
+<Sidebar::SectionLink
-  <Sidebar::SectionLink
+  @badgeText={{@sectionLink.badgeText}}
-    @shouldDisplay={{@sectionLink.shouldDisplay}}
+  @content={{replace-emoji @sectionLink.text}}
-    @linkName={{@sectionLink.name}}
+  @currentWhen={{@sectionLink.currentWhen}}
-    @content={{replace-emoji @sectionLink.text}}
+  @href={{or @sectionLink.href @sectionLink.value}}
-    @prefixType="icon"
+  @linkName={{@sectionLink.name}}
-    @prefixValue={{@sectionLink.prefixValue}}
+  @model={{@sectionLink.model}}
-    @fullReload={{@sectionLink.fullReload}}
+  @models={{@sectionLink.models}}
-    @href={{@sectionLink.value}}
+  @prefixType="icon"
-  />
+  @prefixValue={{@sectionLink.prefixValue}}
-{{else}}
+  @query={{@sectionLink.query}}
-  <Sidebar::SectionLink
+  @route={{@sectionLink.route}}
-    @shouldDisplay={{@sectionLink.shouldDisplay}}
+  @shouldDisplay={{@sectionLink.shouldDisplay}}
-    @href={{@sectionLink.href}}
+  @suffixCSSClass={{@sectionLink.suffixCSSClass}}
-    @title={{@sectionLink.title}}
+  @suffixType={{@sectionLink.suffixType}}
-    @linkName={{@sectionLink.name}}
+  @suffixValue={{@sectionLink.suffixValue}}
-    @route={{@sectionLink.route}}
+  @title={{@sectionLink.title}}
-    @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}}

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 this.currentUser?.user_option?.external_links_in_new_tab &&
-      return "_self";
+      this.isExternal
-    }
-    return this.currentUser?.user_option?.external_links_in_new_tab
       ? "_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];

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() {
+  get route() {}
-    this._notImplemented();
-  }
 
   /**
    * @returns {string} href attribute for the link. This property will take precedence over the `route` property when set.

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;
       }

app/assets/javascripts/discourse/app/lib/sidebar/route-info-helper.js

@@ -1,36 +0,0 @@
-export default class RouteInfoHelper {
-  constructor(router, url) {
-    this.routeInfo = router.recognize(router.rootURL.replace(/\/$/, "") + url);
-  }
-
-  get route() {
-    return this.routeInfo.name;
-  }
-
-  get models() {
-    return this.#getParameters;
-  }
-
-  get query() {
-    return this.routeInfo.queryParams;
-  }
-
-  /**
-   * Extracted from https://github.com/emberjs/rfcs/issues/658
-   * Retrieves all parameters for a `RouteInfo` object and its parents in
-   * correct oder, so that you can pass them to e.g.
-   * `transitionTo(routeName, ...params)`.
-   */
-  get #getParameters() {
-    let allParameters = [];
-    let current = this.routeInfo;
-
-    do {
-      const { params, paramNames } = current;
-      const currentParameters = paramNames.map((n) => params[n]);
-      allParameters = [...currentParameters, ...allParameters];
-    } while ((current = current.parent));
-
-    return allParameters;
-  }
-}

app/assets/javascripts/discourse/app/lib/sidebar/section-link.js

@@ -1,17 +1,10 @@
 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(
+  constructor({ external, icon, id, name, value }, section) {
-    { external, full_reload, icon, id, name, value },
-    section,
-    router
-  ) {
     this.external = external;
-    this.fullReload = full_reload;
     this.prefixValue = icon;
     this.id = id;
     this.name = name;
@@ -19,26 +12,9 @@ export default class SectionLink {
     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;
-  }
 }

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 @@ class SidebarUrl < ActiveRecord::Base
   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

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

spec/system/custom_sidebar_sections_spec.rb

@@ -55,7 +55,7 @@ describe "Custom sidebar sections", type: :system do
     expect(sidebar).to have_section_link("My preferences", target: "_self")
   end
 
-  it "allows the user to create custom section with `/` path which generates a link based on the first item in the `top_menu` site settings" do
+  it "allows the user to create custom section with `/` path" do
     SiteSetting.top_menu = "read|posted|latest"
 
     sign_in user
@@ -67,7 +67,10 @@ describe "Custom sidebar sections", type: :system do
     section_modal.save
 
     expect(sidebar).to have_section("My section")
-    expect(sidebar).to have_section_link("Home", href: "/read")
+    expect(sidebar).to have_section_link("Home", href: "/")
+
+    sidebar.click_section_link("Home")
+    expect(page).to have_css("#navigation-bar .active a[href='/read']")
   end
 
   it "allows the user to create custom section with /pub link" do
@@ -94,9 +97,6 @@ describe "Custom sidebar sections", type: :system do
 
     section_modal.fill_name("My section")
 
-    section_modal.fill_link("Discourse Homepage", "htt")
-    expect(section_modal).to have_disabled_save
-
     section_modal.fill_link("Discourse Homepage", "https://discourse.org")
     expect(section_modal).to have_enabled_save
 
@@ -124,7 +124,41 @@ describe "Custom sidebar sections", type: :system do
     section_modal.save
 
     expect(sidebar).to have_section("My section")
-    expect(sidebar).to have_section_link("Faq", target: "_blank")
+    expect(sidebar).to have_section_link("Faq", target: "_self", href: "/faq#anchor")
+  end
+
+  it "allows the user to create custom section with query param" do
+    sign_in user
+    visit("/latest")
+    sidebar.click_add_section_button
+
+    expect(section_modal).to be_visible
+    expect(section_modal).to have_disabled_save
+    expect(sidebar.custom_section_modal_title).to have_content("Add custom section")
+
+    section_modal.fill_name("My section")
+    section_modal.fill_link("Faq", "/faq?a=b")
+    section_modal.save
+
+    expect(sidebar).to have_section("My section")
+    expect(sidebar).to have_section_link("Faq", target: "_self", href: "/faq?a=b")
+  end
+
+  it "allows the user to create custom section with anchor link" do
+    sign_in user
+    visit("/latest")
+    sidebar.click_add_section_button
+
+    expect(section_modal).to be_visible
+    expect(section_modal).to have_disabled_save
+    expect(sidebar.custom_section_modal_title).to have_content("Add custom section")
+
+    section_modal.fill_name("My section")
+    section_modal.fill_link("Faq", "/faq#someheading")
+    section_modal.save
+
+    expect(sidebar).to have_section("My section")
+    expect(sidebar).to have_section_link("Faq", target: "_self", href: "/faq#someheading")
   end
 
   it "accessibility - when new row is added in custom section, first new input is focused" do
@@ -309,7 +343,7 @@ describe "Custom sidebar sections", type: :system do
     sidebar.click_add_section_button
 
     section_modal.fill_name("A" * (SidebarSection::MAX_TITLE_LENGTH + 1))
-    section_modal.fill_link("B" * (SidebarUrl::MAX_NAME_LENGTH + 1), "/wrong-url")
+    section_modal.fill_link("B" * (SidebarUrl::MAX_NAME_LENGTH + 1), "https:")
 
     expect(page.find(".title.warning")).to have_content("Title must be shorter than 30 characters")
     expect(page.find(".name.warning")).to have_content("Name must be shorter than 80 characters")