DEV: Various changes to support nested posts experiment (#37785)

This PR adds several reusable pieces to Core that the
discourse-nested-replies
plugin needs, while keeping them general-purpose enough to be useful to
other
  plugins and future Core work.

  ## Summary

  ### TopicMetadata component extraction
  - Extracts the category chooser, tag chooser, plugin outlets, and
    save/cancel controls from `topic.gjs` into a standalone
    `<TopicCategoryTagEditor>` component
  - The nested-replies plugin reuses this component in its own topic
    header, avoiding duplication of ~55 lines of template code
  - No behavioral change to the existing topic page

  ### Value transformers for URL routing
  - **`route-to-url`** — applied in `DiscourseURL.routeTo()`, allows
    plugins to intercept and rewrite navigation URLs before routing
    occurs. Returns early if the transformer nullifies the path.
- **`topic-url-for-post-number`** — applied in `Topic#urlForPostNumber`,
    allows plugins to rewrite per-post URLs (e.g. for nested views)
- **`post-share-url`** 

  ### TopicView `PostDependentCache` concern
  - Introduces `memoize_for_posts` — a declarative way to register
    instance variables that should be cleared when posts are replaced
  - Provides `reset_post_collection(posts:)` — an explicit method for
    swapping the post collection on a TopicView after initialization,
    automatically clearing all registered caches
  - Adds `skip_post_loading` initializer option so plugins that supply
    their own posts can skip the default post-loading SQL entirely
  - 4 specs covering replacement, cache clearing, preload hooks, and
    skip_post_loading

---------

Co-authored-by: Isaac Janzen <isaac.janzen@discourse.org>
This commit is contained in:
Mark VanLandingham
2026-03-09 15:47:03 -05:00
committed by GitHub
co-authored by Isaac Janzen
parent c632bb2817
commit ff700ad8c3
13 changed files with 465 additions and 82 deletions
+1 -1
View File
@@ -187,7 +187,7 @@ class TopicViewSerializer < ApplicationSerializer
end
def include_has_deleted?
object.guardian.can_see_deleted_posts?
!object.skip_post_loading && object.guardian.can_see_deleted_posts?
end
def expandable_first_post
@@ -0,0 +1,66 @@
import { hash } from "@ember/helper";
import DButton from "discourse/components/d-button";
import PluginOutlet from "discourse/components/plugin-outlet";
import lazyHash from "discourse/helpers/lazy-hash";
import CategoryChooser from "discourse/select-kit/components/category-chooser";
import MiniTagChooser from "discourse/select-kit/components/mini-tag-chooser";
<template>
{{#if @showCategoryChooser}}
<div class="edit-category__wrapper">
<PluginOutlet
@name="edit-topic-category"
@outletArgs={{lazyHash model=@model buffered=@buffered}}
>
<CategoryChooser
@value={{@buffered.category_id}}
@onChange={{@topicCategoryChanged}}
class="small"
/>
</PluginOutlet>
</div>
{{/if}}
{{#if @canEditTags}}
<div class="edit-tags__wrapper">
<PluginOutlet
@name="edit-topic-tags"
@outletArgs={{lazyHash model=@model buffered=@buffered}}
>
<MiniTagChooser
@value={{@buffered.tags}}
@onChange={{@topicTagsChanged}}
@options={{hash
filterable=true
categoryId=@buffered.category_id
minimum=@minimumRequiredTags
filterPlaceholder="tagging.choose_for_topic"
useHeaderFilter=true
}}
/>
</PluginOutlet>
</div>
{{/if}}
<PluginOutlet
@name="edit-topic"
@connectorTagName="div"
@outletArgs={{lazyHash model=@model buffered=@buffered}}
/>
<div class="edit-controls">
<DButton
@action={{@onSave}}
@icon="check"
@ariaLabel="composer.save_edit"
class="btn-primary submit-edit"
/>
<DButton
@action={{@onCancel}}
@icon="xmark"
@ariaLabel="composer.cancel"
class="btn-default cancel-edit"
/>
{{yield}}
</div>
</template>
@@ -88,6 +88,7 @@ export const VALUE_TRANSFORMERS = Object.freeze([
"post-meta-data-infos",
"post-meta-data-poster-name-suppress-similar-name",
"post-notice-component",
"post-share-url",
"post-show-topic-map",
"post-small-action-class",
"post-small-action-custom-component",
@@ -97,6 +98,7 @@ export const VALUE_TRANSFORMERS = Object.freeze([
"poster-name-user-title",
"preferences-save-attributes",
"quote-params",
"route-to-url",
"small-user-attrs",
"tag-separator",
"topic-list-class",
@@ -106,6 +108,7 @@ export const VALUE_TRANSFORMERS = Object.freeze([
"topic-list-item-expand-pinned",
"topic-list-item-mobile-layout",
"topic-list-item-style",
"topic-url-for-post-number",
"user-field-components",
"user-menu-notification-item-acting-user-avatar",
"user-notes-modal-subtitle",
+6
View File
@@ -9,6 +9,7 @@ import { isTesting } from "discourse/lib/environment";
import getURL, { withoutPrefix } from "discourse/lib/get-url";
import LockOn from "discourse/lib/lock-on";
import offsetCalculator from "discourse/lib/offset-calculator";
import { applyValueTransformer } from "discourse/lib/transformer";
import { defaultHomepage } from "discourse/lib/utilities";
import Category from "discourse/models/category";
import Session from "discourse/models/session";
@@ -211,6 +212,11 @@ class DiscourseURL extends EmberObject {
return;
}
path = applyValueTransformer("route-to-url", path, { opts });
if (isEmpty(path)) {
return;
}
// In embed mode, open all navigation in new tabs except same-topic navigation
if (EmbedMode.enabled) {
const currentTopicMatch = TOPIC_URL_REGEXP.exec(window.location.pathname);
+3 -1
View File
@@ -14,6 +14,7 @@ import {
defineTrackedProperty,
enumerateTrackedKeys,
} from "discourse/lib/tracked-tools";
import { applyValueTransformer } from "discourse/lib/transformer";
import { userPath } from "discourse/lib/url";
import { postUrl } from "discourse/lib/utilities";
import ActionSummary from "discourse/models/action-summary";
@@ -219,7 +220,8 @@ export default class Post extends RestModel {
}
get shareUrl() {
return this.customShare || resolveShareUrl(this.url, this.currentUser);
const url = this.customShare || resolveShareUrl(this.url, this.currentUser);
return applyValueTransformer("post-share-url", url, { post: this });
}
@computed("name", "username")
+5 -1
View File
@@ -22,6 +22,7 @@ import {
autoTrackedArray,
defineTrackedProperty,
} from "discourse/lib/tracked-tools";
import { applyValueTransformer } from "discourse/lib/transformer";
import DiscourseURL, { userPath } from "discourse/lib/url";
import ActionSummary from "discourse/models/action-summary";
import Bookmark from "discourse/models/bookmark";
@@ -581,7 +582,10 @@ export default class Topic extends RestModel {
if (postNumber > 0) {
url += `/${postNumber}`;
}
return url;
return applyValueTransformer("topic-url-for-post-number", url, {
topic: this,
postNumber,
});
}
@computed("unread_posts", "new_posts")
+13 -68
View File
@@ -27,6 +27,7 @@ import TopicCategory from "discourse/components/topic-category";
import TopicFooterButtons from "discourse/components/topic-footer-buttons";
import TopicLocalizedContentToggle from "discourse/components/topic-localized-content-toggle";
import TopicMap from "discourse/components/topic-map/index";
import TopicMetadata from "discourse/components/topic-metadata";
import TopicNavigation from "discourse/components/topic-navigation";
import TopicProgress from "discourse/components/topic-progress";
import TopicSkipLinks from "discourse/components/topic-skip-links";
@@ -42,8 +43,6 @@ import hideApplicationFooter from "discourse/helpers/hide-application-footer";
import hideScrollableContent from "discourse/helpers/hide-scrollable-content";
import lazyHash from "discourse/helpers/lazy-hash";
import routeAction from "discourse/helpers/route-action";
import CategoryChooser from "discourse/select-kit/components/category-chooser";
import MiniTagChooser from "discourse/select-kit/components/mini-tag-chooser";
import { and, eq } from "discourse/truth-helpers";
import { i18n } from "discourse-i18n";
import booleanString from "../helpers/boolean-string";
@@ -121,71 +120,17 @@ export default <template>
@buffered={{@controller.buffered}}
/>
{{#if @controller.showCategoryChooser}}
<div class="edit-category__wrapper">
<PluginOutlet
@name="edit-topic-category"
@outletArgs={{lazyHash
model=@controller.model
buffered=@controller.buffered
}}
>
<CategoryChooser
@value={{@controller.buffered.category_id}}
@onChange={{@controller.topicCategoryChanged}}
class="small"
/>
</PluginOutlet>
</div>
{{/if}}
{{#if @controller.canEditTags}}
<div class="edit-tags__wrapper">
<PluginOutlet
@name="edit-topic-tags"
@outletArgs={{lazyHash
model=@controller.model
buffered=@controller.buffered
}}
>
<MiniTagChooser
@value={{@controller.buffered.tags}}
@onChange={{@controller.topicTagsChanged}}
@options={{hash
filterable=true
categoryId=@controller.buffered.category_id
minimum=@controller.minimumRequiredTags
filterPlaceholder="tagging.choose_for_topic"
useHeaderFilter=true
}}
/>
</PluginOutlet>
</div>
{{/if}}
<PluginOutlet
@name="edit-topic"
@connectorTagName="div"
@outletArgs={{lazyHash
model=@controller.model
buffered=@controller.buffered
}}
/>
<div class="edit-controls">
<DButton
@action={{@controller.finishedEditingTopic}}
@icon="check"
@ariaLabel="composer.save_edit"
class="btn-primary submit-edit"
/>
<DButton
@action={{@controller.cancelEditingTopic}}
@icon="xmark"
@ariaLabel="composer.cancel"
class="btn-default cancel-edit"
/>
<TopicMetadata
@buffered={{@controller.buffered}}
@model={{@controller.model}}
@showCategoryChooser={{@controller.showCategoryChooser}}
@canEditTags={{@controller.canEditTags}}
@minimumRequiredTags={{@controller.minimumRequiredTags}}
@onSave={{@controller.finishedEditingTopic}}
@onCancel={{@controller.cancelEditingTopic}}
@topicCategoryChanged={{@controller.topicCategoryChanged}}
@topicTagsChanged={{@controller.topicTagsChanged}}
>
{{#if @controller.canRemoveTopicFeaturedLink}}
<a
href
@@ -197,7 +142,7 @@ export default <template>
{{@controller.featuredLinkDomain}}
</a>
{{/if}}
</div>
</TopicMetadata>
</div>
{{else}}
@@ -0,0 +1,51 @@
import { getOwner } from "@ember/owner";
import { visit } from "@ember/test-helpers";
import { test } from "qunit";
import { withPluginApi } from "discourse/lib/plugin-api";
import { acceptance } from "discourse/tests/helpers/qunit-helpers";
acceptance("post-share-url transformer", function (needs) {
needs.user();
test("applying a value transformation", async function (assert) {
withPluginApi((api) => {
api.registerValueTransformer("post-share-url", ({ value }) => {
return "/custom" + value;
});
});
await visit("/t/internationalization-localization/280");
const topicController = getOwner(this).lookup("controller:topic");
const post = topicController.model.postStream.posts[0];
assert.true(
post.shareUrl.startsWith("/custom"),
"it transforms the share url"
);
});
test("transformer receives the post in context", async function (assert) {
let receivedPost;
withPluginApi((api) => {
api.registerValueTransformer("post-share-url", ({ value, context }) => {
receivedPost = context.post;
return value;
});
});
await visit("/t/internationalization-localization/280");
const topicController = getOwner(this).lookup("controller:topic");
const post = topicController.model.postStream.posts[0];
// access shareUrl to trigger the transformer
post.shareUrl;
assert.strictEqual(
receivedPost.post_number,
1,
"transformer receives the correct post in context"
);
});
});
@@ -0,0 +1,58 @@
import { getOwner } from "@ember/owner";
import { visit } from "@ember/test-helpers";
import { test } from "qunit";
import { withPluginApi } from "discourse/lib/plugin-api";
import { acceptance } from "discourse/tests/helpers/qunit-helpers";
acceptance("topic-url-for-post-number transformer", function (needs) {
needs.user();
test("applying a value transformation", async function (assert) {
withPluginApi((api) => {
api.registerValueTransformer("topic-url-for-post-number", ({ value }) => {
return "/custom" + value;
});
});
await visit("/t/internationalization-localization/280");
const topicController = getOwner(this).lookup("controller:topic");
const topic = topicController.model;
assert.true(
topic.urlForPostNumber(1).startsWith("/custom"),
"it transforms the topic url for post number"
);
});
test("transformer receives the topic and postNumber in context", async function (assert) {
let receivedContext;
withPluginApi((api) => {
api.registerValueTransformer(
"topic-url-for-post-number",
({ value, context }) => {
receivedContext = context;
return value;
}
);
});
await visit("/t/internationalization-localization/280");
const topicController = getOwner(this).lookup("controller:topic");
const topic = topicController.model;
topic.urlForPostNumber(3);
assert.strictEqual(
receivedContext.topic,
topic,
"transformer receives the topic in context"
);
assert.strictEqual(
receivedContext.postNumber,
3,
"transformer receives the postNumber in context"
);
});
});
@@ -2,6 +2,7 @@ import { setupTest } from "ember-qunit";
import { module, test } from "qunit";
import sinon from "sinon";
import { setPrefix } from "discourse/lib/get-url";
import { withPluginApi } from "discourse/lib/plugin-api";
import DiscourseURL, {
getCanonicalUrl,
getCategoryAndTagUrl,
@@ -192,6 +193,75 @@ module("Unit | Utility | url", function (hooks) {
);
});
test("routeTo applies route-to-url value transformer", async function (assert) {
sinon.stub(DiscourseURL, "origin").get(() => "http://example.com");
sinon.stub(DiscourseURL, "handleURL");
sinon.stub(DiscourseURL, "router").get(() => {
return { currentURL: "/bar" };
});
withPluginApi((api) => {
api.registerValueTransformer("route-to-url", ({ value }) => {
if (value === "/t/some-topic/123") {
return "/nested/some-topic/123";
}
return value;
});
});
DiscourseURL.routeTo("/t/some-topic/123");
assert.true(
DiscourseURL.handleURL.calledWith("/nested/some-topic/123"),
"the transformed URL is used for routing"
);
});
test("routeTo passes untransformed URLs through", async function (assert) {
sinon.stub(DiscourseURL, "origin").get(() => "http://example.com");
sinon.stub(DiscourseURL, "handleURL");
sinon.stub(DiscourseURL, "router").get(() => {
return { currentURL: "/bar" };
});
withPluginApi((api) => {
api.registerValueTransformer("route-to-url", ({ value }) => {
if (value.startsWith("/t/redirect-me")) {
return "/other/path";
}
return value;
});
});
DiscourseURL.routeTo("/some/other/path");
assert.true(
DiscourseURL.handleURL.calledWith("/some/other/path"),
"non-matching URLs pass through unchanged"
);
});
test("routeTo aborts when transformer returns null", async function (assert) {
sinon.stub(DiscourseURL, "origin").get(() => "http://example.com");
sinon.stub(DiscourseURL, "handleURL");
sinon.stub(DiscourseURL, "redirectTo");
sinon.stub(DiscourseURL, "router").get(() => {
return { currentURL: "/bar" };
});
withPluginApi((api) => {
api.registerValueTransformer("route-to-url", () => null);
});
DiscourseURL.routeTo("/t/some-topic/123");
assert.false(
DiscourseURL.handleURL.called,
"handleURL is not called when transformer returns null"
);
assert.false(
DiscourseURL.redirectTo.called,
"redirectTo is not called when transformer returns null"
);
});
test("prefixProtocol", async function (assert) {
assert.strictEqual(
prefixProtocol("mailto:mr-beaver@aol.com"),
+32 -11
View File
@@ -4,6 +4,20 @@ class TopicView
MEGA_TOPIC_POSTS_COUNT = 10_000
MIN_POST_READ_TIME = 4.0
include PostDependentCache
memoize_for_posts :all_post_actions
memoize_for_posts :reviewable_counts
memoize_for_posts :post_custom_fields
memoize_for_posts :user_custom_fields
memoize_for_posts :category_group_moderator_user_ids
memoize_for_posts :mentioned_users
memoize_for_posts :link_counts
memoize_for_posts :read_posts_set
memoize_for_posts :primary_group_names, :@group_names
memoize_for_posts :post_user_badges
memoize_for_posts :last_post
def self.on_preload(&blk)
(@preload ||= Set.new) << blk
end
@@ -31,6 +45,7 @@ class TopicView
:personal_message,
:can_review_topic,
:page,
:skip_post_loading,
)
alias queued_posts_enabled? queued_posts_enabled
@@ -131,22 +146,28 @@ class TopicView
@page = @page.to_i > 1 ? @page.to_i : calculate_page
setup_filtered_posts
@filtered_posts = apply_default_scope(@filtered_posts)
filter_posts(options)
if @skip_post_loading
@filtered_posts = @topic.posts.none
@predelete_filtered_posts = @filtered_posts
@posts = []
else
setup_filtered_posts
@filtered_posts = apply_default_scope(@filtered_posts)
filter_posts(options)
if @posts && !@skip_custom_fields
if (added_fields = User.allowed_user_custom_fields(@guardian)).present?
@user_custom_fields = User.custom_fields_for_ids(@posts.map(&:user_id), added_fields)
if @posts && !@skip_custom_fields
if (added_fields = User.allowed_user_custom_fields(@guardian)).present?
@user_custom_fields = User.custom_fields_for_ids(@posts.map(&:user_id), added_fields)
end
if (allowed_fields = TopicView.allowed_post_custom_fields(@user, @topic)).present?
@post_custom_fields = Post.custom_fields_for_ids(@posts.map(&:id), allowed_fields)
end
end
if (allowed_fields = TopicView.allowed_post_custom_fields(@user, @topic)).present?
@post_custom_fields = Post.custom_fields_for_ids(@posts.map(&:id), allowed_fields)
end
TopicView.preload(self)
end
TopicView.preload(self)
@draft_key = @topic.draft_key
@draft_sequence = DraftSequence.current(@user, @draft_key)
+43
View File
@@ -0,0 +1,43 @@
# frozen_string_literal: true
# Extracted from TopicView to make post-dependent memoization self-maintaining.
#
# Any method in TopicView (or a plugin) that caches data derived from @posts
# can register itself with `memoize_for_posts`. When @posts is replaced via
# `reset_post_collection`, all registered caches are automatically cleared —
# no hardcoded ivar list required.
module TopicView::PostDependentCache
extend ActiveSupport::Concern
included do
# Stores ivar names (as symbols like :@all_post_actions) that should
# be cleared whenever @posts is replaced.
class_attribute :post_dependent_ivars, instance_writer: false, default: []
end
class_methods do
# Register a memoized ivar as post-dependent.
#
# memoize_for_posts :all_post_actions
# memoize_for_posts :primary_group_names, :@group_names
#
# The ivar name defaults to `@<method_name>` but can be overridden
# for cases where the ivar doesn't match the method name.
def memoize_for_posts(method_name, ivar_name = nil)
ivar_name ||= :"@#{method_name}"
# Use += to avoid mutating a parent class's array
self.post_dependent_ivars = post_dependent_ivars + [ivar_name]
end
end
# Replaces @posts with a new collection and clears all registered
# post-dependent caches. Use this instead of writing to @posts directly
# when you need to swap in a different set of posts (e.g. a plugin that
# builds its own post tree) after the TopicView has been initialized.
def reset_post_collection(posts:)
@posts = posts
self.class.post_dependent_ivars.each do |ivar|
remove_instance_variable(ivar) if instance_variable_defined?(ivar)
end
end
end
+114
View File
@@ -28,6 +28,120 @@ RSpec.describe TopicView do
end
end
describe "#reset_post_collection" do
fab!(:post1) { Fabricate(:post, topic: topic) }
fab!(:post2) { Fabricate(:post, topic: topic) }
fab!(:post3) { Fabricate(:post, topic: topic) }
it "replaces the posts collection" do
tv = TopicView.new(topic.id, evil_trout)
original_posts = tv.posts.to_a
new_posts = [post3]
tv.reset_post_collection(posts: new_posts)
expect(tv.posts).to eq(new_posts)
expect(tv.posts).not_to eq(original_posts)
end
it "clears memoized state derived from the previous posts" do
tv = TopicView.new(topic.id, evil_trout)
tv.all_post_actions
tv.reviewable_counts
tv.mentioned_users
tv.category_group_moderator_user_ids
tv.primary_group_names
tv.last_post
tv.reset_post_collection(posts: [post2])
expect(tv.all_post_actions).to be_a(Hash)
expect(tv.last_post).to eq(post2)
expect(tv.mentioned_users).to eq({})
expect(tv.primary_group_names).to be_a(Hash)
end
it "allows preload hooks to run on the new posts" do
tv = TopicView.new(topic.id, evil_trout)
preloaded_post_ids = nil
preloader = lambda { |view| preloaded_post_ids = view.posts.map(&:id) }
TopicView.on_preload(&preloader)
tv.reset_post_collection(posts: [post2, post3])
TopicView.preload(tv)
expect(preloaded_post_ids).to contain_exactly(post2.id, post3.id)
ensure
TopicView.cancel_preload(&preloader)
end
it "skips post loading when skip_post_loading is true" do
tv = TopicView.new(topic.id, evil_trout, skip_post_loading: true)
expect(tv.posts).to eq([])
expect(tv.filtered_posts.count).to eq(0)
expect(tv.topic).to eq(topic)
tv.reset_post_collection(posts: [post1, post2])
expect(tv.posts).to eq([post1, post2])
end
end
describe "#reset_post_collection (memoize_for_posts)" do
fab!(:post1) { Fabricate(:post, topic: topic) }
fab!(:post2) { Fabricate(:post, topic: topic) }
it "clears all registered post-dependent caches when posts are replaced" do
tv = TopicView.new(topic.id, evil_trout)
# Force memoization of a registered cache
tv.all_post_actions
expect(tv.instance_variable_defined?(:@all_post_actions)).to eq(true)
tv.reset_post_collection(posts: [post1])
expect(tv.instance_variable_defined?(:@all_post_actions)).to eq(false)
end
it "clears caches with custom ivar names" do
tv = TopicView.new(topic.id, evil_trout)
# primary_group_names is registered as `memoize_for_posts :primary_group_names, :@group_names`
tv.primary_group_names
expect(tv.instance_variable_defined?(:@group_names)).to eq(true)
tv.reset_post_collection(posts: [post1])
expect(tv.instance_variable_defined?(:@group_names)).to eq(false)
end
it "allows plugins to register their own post-dependent caches" do
original_ivars = TopicView.post_dependent_ivars.dup
TopicView.memoize_for_posts(:test_plugin_cache)
tv = TopicView.new(topic.id, evil_trout)
tv.instance_variable_set(:@test_plugin_cache, { some: "data" })
tv.reset_post_collection(posts: [post1])
expect(tv.instance_variable_defined?(:@test_plugin_cache)).to eq(false)
ensure
TopicView.post_dependent_ivars = original_ivars
end
it "replaces @posts with the new collection" do
tv = TopicView.new(topic.id, evil_trout)
tv.reset_post_collection(posts: [post2])
expect(tv.posts).to eq([post2])
tv.reset_post_collection(posts: [post1, post2])
expect(tv.posts).to eq([post1, post2])
end
end
it "raises a not found error if the topic doesn't exist" do
expect { TopicView.new(1_231_232, evil_trout) }.to raise_error(Discourse::NotFound)
end