Avoid looking up settings during timeline rendering (#8313)

* Avoid showHiddenEventsInTimeline lookups

* Avoid MSC3531 feature lookups

* Test that showHiddenEventsInTimeline doesn't get looked up while
rendering

* Fix code review nits

Co-authored-by: Travis Ralston <travisr@matrix.org>

src/Unread.ts

@@ -48,7 +48,7 @@ export function eventTriggersUnreadCount(ev: MatrixEvent): boolean {
     }
 
     if (ev.isRedacted()) return false;
-    return haveRendererForEvent(ev);
+    return haveRendererForEvent(ev, false /* hidden messages should never trigger unread counts anyways */);
 }
 
 export function doesRoomHaveUnreadMessages(room: Room): boolean {

src/components/structures/MessagePanel.tsx

@@ -242,7 +242,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
     // displayed event in the current render cycle.
     private readReceiptsByUserId: Record<string, IReadReceiptForUser> = {};
 
-    private readonly showHiddenEventsInTimeline: boolean;
+    private readonly _showHiddenEvents: boolean;
     private readonly threadsEnabled: boolean;
     private isMounted = false;
 
@@ -270,7 +270,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
         // Cache these settings on mount since Settings is expensive to query,
         // and we check this in a hot code path. This is also cached in our
         // RoomContext, however we still need a fallback for roomless MessagePanels.
-        this.showHiddenEventsInTimeline = SettingsStore.getValue("showHiddenEventsInTimeline");
+        this._showHiddenEvents = SettingsStore.getValue("showHiddenEventsInTimeline");
         this.threadsEnabled = SettingsStore.getValue("feature_thread");
 
         this.showTypingNotificationsWatcherRef =
@@ -465,7 +465,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
     };
 
     public get showHiddenEvents(): boolean {
-        return this.context?.showHiddenEventsInTimeline ?? this.showHiddenEventsInTimeline;
+        return this.context?.showHiddenEvents ?? this._showHiddenEvents;
     }
 
     // TODO: Implement granular (per-room) hide options
@@ -748,7 +748,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
             const willWantDateSeparator = this.wantsDateSeparator(mxEv, nextEv.getDate() || new Date());
             lastInSection = willWantDateSeparator ||
                 mxEv.getSender() !== nextEv.getSender() ||
-                getEventDisplayInfo(nextEv).isInfoMessage ||
+                getEventDisplayInfo(nextEv, this.showHiddenEvents).isInfoMessage ||
                 !shouldFormContinuation(
                     mxEv, nextEv, this.showHiddenEvents, this.threadsEnabled, this.context.timelineRenderingType,
                 );

src/components/structures/RoomView.tsx

@@ -199,7 +199,7 @@ export interface IRoomState {
     showTwelveHourTimestamps: boolean;
     readMarkerInViewThresholdMs: number;
     readMarkerOutOfViewThresholdMs: number;
-    showHiddenEventsInTimeline: boolean;
+    showHiddenEvents: boolean;
     showReadReceipts: boolean;
     showRedactions: boolean;
     showJoinLeaves: boolean;
@@ -271,7 +271,7 @@ export class RoomView extends React.Component<IRoomProps, IRoomState> {
             showTwelveHourTimestamps: SettingsStore.getValue("showTwelveHourTimestamps"),
             readMarkerInViewThresholdMs: SettingsStore.getValue("readMarkerInViewThresholdMs"),
             readMarkerOutOfViewThresholdMs: SettingsStore.getValue("readMarkerOutOfViewThresholdMs"),
-            showHiddenEventsInTimeline: SettingsStore.getValue("showHiddenEventsInTimeline"),
+            showHiddenEvents: SettingsStore.getValue("showHiddenEventsInTimeline"),
             showReadReceipts: true,
             showRedactions: true,
             showJoinLeaves: true,
@@ -328,7 +328,7 @@ export class RoomView extends React.Component<IRoomProps, IRoomState> {
                 this.setState({ readMarkerOutOfViewThresholdMs: value as number }),
             ),
             SettingsStore.watchSetting("showHiddenEventsInTimeline", null, (...[,,, value]) =>
-                this.setState({ showHiddenEventsInTimeline: value as boolean }),
+                this.setState({ showHiddenEvents: value as boolean }),
             ),
         ];
     }
@@ -1480,7 +1480,7 @@ export class RoomView extends React.Component<IRoomProps, IRoomState> {
                 continue;
             }
 
-            if (!haveRendererForEvent(mxEv, this.state.showHiddenEventsInTimeline)) {
+            if (!haveRendererForEvent(mxEv, this.state.showHiddenEvents)) {
                 // XXX: can this ever happen? It will make the result count
                 // not match the displayed count.
                 continue;

src/components/structures/ThreadPanel.tsx

@@ -250,7 +250,7 @@ const ThreadPanel: React.FC<IProps> = ({
         <RoomContext.Provider value={{
             ...roomContext,
             timelineRenderingType: TimelineRenderingType.ThreadsList,
-            showHiddenEventsInTimeline: true,
+            showHiddenEvents: true,
             narrow,
         }}>
             <BaseCard

src/components/structures/TimelinePanel.tsx

@@ -1511,7 +1511,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
 
             const shouldIgnore = !!ev.status || // local echo
                 (ignoreOwn && ev.getSender() === myUserId); // own message
-            const isWithoutTile = !haveRendererForEvent(ev, this.context?.showHiddenEventsInTimeline) ||
+            const isWithoutTile = !haveRendererForEvent(ev, this.context?.showHiddenEvents) ||
                 shouldHideEvent(ev, this.context);
 
             if (isWithoutTile || !node) {

src/components/views/messages/TextualEvent.tsx

@@ -28,7 +28,7 @@ export default class TextualEvent extends React.Component<IProps> {
     static contextType = RoomContext;
 
     public render() {
-        const text = TextForEvent.textForEvent(this.props.mxEvent, true, this.context?.showHiddenEventsInTimeline);
+        const text = TextForEvent.textForEvent(this.props.mxEvent, true, this.context?.showHiddenEvents);
         if (!text) return null;
         return <div className="mx_TextualEvent">{ text }</div>;
     }

src/components/views/rooms/EventTile.tsx

@@ -1214,9 +1214,12 @@ export class UnwrappedEventTile extends React.Component<IProps, IState> {
             msgOption = readAvatars;
         }
 
-        const replyChain =
+        let replyChain;
-            (haveRendererForEvent(this.props.mxEvent) && shouldDisplayReply(this.props.mxEvent))
+        if (
-                ? <ReplyChain
+            haveRendererForEvent(this.props.mxEvent, this.context.showHiddenEvents) &&
+            shouldDisplayReply(this.props.mxEvent)
+        ) {
+            replyChain = <ReplyChain
                 parentEv={this.props.mxEvent}
                 onHeightChanged={this.props.onHeightChanged}
                 ref={this.replyChain}
@@ -1227,8 +1230,8 @@ export class UnwrappedEventTile extends React.Component<IProps, IState> {
                 isQuoteExpanded={isQuoteExpanded}
                 setQuoteExpanded={this.setQuoteExpanded}
                 getRelationsForEvent={this.props.getRelationsForEvent}
-                />
+            />;
-                : null;
+        }
 
         const isOwnEvent = this.props.mxEvent?.sender?.userId === MatrixClientPeg.get().getUserId();
 
@@ -1267,7 +1270,7 @@ export class UnwrappedEventTile extends React.Component<IProps, IState> {
                             highlightLink: this.props.highlightLink,
                             onHeightChanged: this.props.onHeightChanged,
                             permalinkCreator: this.props.permalinkCreator,
-                        }) }
+                        }, this.context.showHiddenEvents) }
                     </div>,
                 ]);
             }
@@ -1309,7 +1312,7 @@ export class UnwrappedEventTile extends React.Component<IProps, IState> {
                             highlightLink: this.props.highlightLink,
                             onHeightChanged: this.props.onHeightChanged,
                             permalinkCreator: this.props.permalinkCreator,
-                        }) }
+                        }, this.context.showHiddenEvents) }
                         { actionBar }
                         <a href={permalink} onClick={this.onPermalinkClicked}>
                             { timestamp }
@@ -1395,7 +1398,7 @@ export class UnwrappedEventTile extends React.Component<IProps, IState> {
                             highlightLink: this.props.highlightLink,
                             onHeightChanged: this.props.onHeightChanged,
                             permalinkCreator: this.props.permalinkCreator,
-                        }) }
+                        }, this.context.showHiddenEvents) }
                     </div>,
                     <a
                         className="mx_EventTile_senderDetailsLink"
@@ -1448,7 +1451,7 @@ export class UnwrappedEventTile extends React.Component<IProps, IState> {
                                 highlightLink: this.props.highlightLink,
                                 onHeightChanged: this.props.onHeightChanged,
                                 permalinkCreator: this.props.permalinkCreator,
-                            }) }
+                            }, this.context.showHiddenEvents) }
                             { keyRequestInfo }
                             { actionBar }
                             { this.props.layout === Layout.IRC && <>

src/components/views/rooms/ReplyTile.tsx

@@ -110,7 +110,9 @@ export default class ReplyTile extends React.PureComponent<IProps> {
         const msgType = mxEvent.getContent().msgtype;
         const evType = mxEvent.getType() as EventType;
 
-        const { hasRenderer, isInfoMessage, isSeeingThroughMessageHiddenForModeration } = getEventDisplayInfo(mxEvent);
+        const {
+            hasRenderer, isInfoMessage, isSeeingThroughMessageHiddenForModeration,
+        } = getEventDisplayInfo(mxEvent, false /* Replies are never hidden, so this should be fine */);
         // This shouldn't happen: the caller should check we support this type
         // before trying to instantiate us
         if (!hasRenderer) {
@@ -177,7 +179,7 @@ export default class ReplyTile extends React.PureComponent<IProps> {
                         highlightLink: this.props.highlightLink,
                         onHeightChanged: this.props.onHeightChanged,
                         permalinkCreator: this.props.permalinkCreator,
-                    }) }
+                    }, false /* showHiddenEvents shouldn't be relevant */) }
                 </a>
             </div>
         );

src/components/views/rooms/SearchResultTile.tsx

@@ -78,7 +78,7 @@ export default class SearchResultTile extends React.Component<IProps> {
                 highlights = this.props.searchHighlights;
             }
 
-            if (haveRendererForEvent(mxEv, this.context?.showHiddenEventsInTimeline)) {
+            if (haveRendererForEvent(mxEv, this.context?.showHiddenEvents)) {
                 // do we need a date separator since the last event?
                 const prevEv = timeline[j - 1];
                 // is this a continuation of the previous message?
@@ -87,7 +87,7 @@ export default class SearchResultTile extends React.Component<IProps> {
                     shouldFormContinuation(
                         prevEv,
                         mxEv,
-                        this.context?.showHiddenEventsInTimeline,
+                        this.context?.showHiddenEvents,
                         threadsEnabled,
                         TimelineRenderingType.Search,
                     );
@@ -102,7 +102,7 @@ export default class SearchResultTile extends React.Component<IProps> {
                         !shouldFormContinuation(
                             mxEv,
                             nextEv,
-                            this.context?.showHiddenEventsInTimeline,
+                            this.context?.showHiddenEvents,
                             threadsEnabled,
                             TimelineRenderingType.Search,
                         )

src/contexts/RoomContext.ts

@@ -54,7 +54,7 @@ const RoomContext = createContext<IRoomState>({
     showTwelveHourTimestamps: false,
     readMarkerInViewThresholdMs: 3000,
     readMarkerOutOfViewThresholdMs: 30000,
-    showHiddenEventsInTimeline: false,
+    showHiddenEvents: false,
     showReadReceipts: true,
     showRedactions: true,
     showJoinLeaves: true,

src/events/EventTileFactory.tsx

@@ -141,19 +141,25 @@ const SINGULAR_STATE_EVENTS = new Set([
  * Find an event tile factory for the given conditions.
  * @param mxEvent The event.
  * @param cli The matrix client to reference when needed.
+ * @param showHiddenEvents Whether hidden events should be shown.
  * @param asHiddenEv When true, treat the event as always hidden.
  * @returns The factory, or falsy if not possible.
  */
-export function pickFactory(mxEvent: MatrixEvent, cli: MatrixClient, asHiddenEv?: boolean): Optional<Factory> {
+export function pickFactory(
+    mxEvent: MatrixEvent,
+    cli: MatrixClient,
+    showHiddenEvents: boolean,
+    asHiddenEv?: boolean,
+): Optional<Factory> {
     const evType = mxEvent.getType(); // cache this to reduce call stack execution hits
 
     // Note: we avoid calling SettingsStore unless absolutely necessary - this code is on the critical path.
 
-    if (asHiddenEv && SettingsStore.getValue("showHiddenEventsInTimeline")) {
+    if (asHiddenEv && showHiddenEvents) {
         return JSONEventFactory;
     }
 
-    const noEventFactoryFactory: (() => Optional<Factory>) = () => SettingsStore.getValue("showHiddenEventsInTimeline")
+    const noEventFactoryFactory: (() => Optional<Factory>) = () => showHiddenEvents
         ? JSONEventFactory
         : undefined; // just don't render things that we shouldn't render
 
@@ -242,17 +248,19 @@ export function pickFactory(mxEvent: MatrixEvent, cli: MatrixClient, asHiddenEv?
  * Render an event as a tile
  * @param renderType The render type. Used to inform properties given to the eventual component.
  * @param props The properties to provide to the eventual component.
+ * @param showHiddenEvents Whether hidden events should be shown.
  * @param cli Optional client instance to use, otherwise the default MatrixClientPeg will be used.
  * @returns The tile as JSX, or falsy if unable to render.
  */
 export function renderTile(
     renderType: TimelineRenderingType,
     props: EventTileTypeProps,
+    showHiddenEvents: boolean,
     cli?: MatrixClient,
 ): Optional<JSX.Element> {
     cli = cli ?? MatrixClientPeg.get(); // because param defaults don't do the correct thing
 
-    const factory = pickFactory(props.mxEvent, cli);
+    const factory = pickFactory(props.mxEvent, cli, showHiddenEvents);
     if (!factory) return undefined;
 
     // Note that we split off the ones we actually care about here just to be sure that we're
@@ -316,16 +324,18 @@ export function renderTile(
 /**
  * A version of renderTile() specifically for replies.
  * @param props The properties to specify on the eventual object.
+ * @param showHiddenEvents Whether hidden events should be shown.
  * @param cli Optional client instance to use, otherwise the default MatrixClientPeg will be used.
  * @returns The tile as JSX, or falsy if unable to render.
  */
 export function renderReplyTile(
     props: EventTileTypeProps,
+    showHiddenEvents: boolean,
     cli?: MatrixClient,
 ): Optional<JSX.Element> {
     cli = cli ?? MatrixClientPeg.get(); // because param defaults don't do the correct thing
 
-    const factory = pickFactory(props.mxEvent, cli);
+    const factory = pickFactory(props.mxEvent, cli, showHiddenEvents);
     if (!factory) return undefined;
 
     // See renderTile() for why we split off so much
@@ -367,7 +377,7 @@ export function isMessageEvent(ev: MatrixEvent): boolean {
     return (messageTypes.includes(ev.getType() as EventType)) || M_POLL_START.matches(ev.getType());
 }
 
-export function haveRendererForEvent(mxEvent: MatrixEvent, showHiddenEvents?: boolean): boolean {
+export function haveRendererForEvent(mxEvent: MatrixEvent, showHiddenEvents: boolean): boolean {
     // Only show "Message deleted" tile for plain message events, encrypted events,
     // and state events as they'll likely still contain enough keys to be relevant.
     if (mxEvent.isRedacted() && !mxEvent.isEncrypted() && !isMessageEvent(mxEvent) && !mxEvent.isState()) {
@@ -377,7 +387,7 @@ export function haveRendererForEvent(mxEvent: MatrixEvent, showHiddenEvents?: bo
     // No tile for replacement events since they update the original tile
     if (mxEvent.isRelation(RelationType.Replace)) return false;
 
-    const handler = pickFactory(mxEvent, MatrixClientPeg.get());
+    const handler = pickFactory(mxEvent, MatrixClientPeg.get(), showHiddenEvents);
     if (!handler) return false;
     if (handler === TextualEventFactory) {
         return hasText(mxEvent, showHiddenEvents);

src/settings/Settings.tsx

@@ -187,6 +187,8 @@ export const SETTINGS: {[setting: string]: ISetting} = {
     "feature_msc3531_hide_messages_pending_moderation": {
         isFeature: true,
         labsGroup: LabGroup.Moderation,
+        // Requires a reload since this setting is cached in EventUtils
+        controller: new ReloadOnChangeController(),
         displayName: _td("Let moderators hide messages pending moderation."),
         supportedLevels: LEVELS_FEATURE,
         default: false,

src/utils/EventRenderingUtils.ts

@@ -25,7 +25,7 @@ import { haveRendererForEvent, JitsiEventFactory, JSONEventFactory, pickFactory
 import { MatrixClientPeg } from "../MatrixClientPeg";
 import { getMessageModerationState, MessageModerationState } from "./EventUtils";
 
-export function getEventDisplayInfo(mxEvent: MatrixEvent, hideEvent?: boolean): {
+export function getEventDisplayInfo(mxEvent: MatrixEvent, showHiddenEvents: boolean, hideEvent?: boolean): {
     isInfoMessage: boolean;
     hasRenderer: boolean;
     isBubbleMessage: boolean;
@@ -52,7 +52,7 @@ export function getEventDisplayInfo(mxEvent: MatrixEvent, hideEvent?: boolean):
     }
 
     // TODO: Thread a MatrixClient through to here
-    let factory = pickFactory(mxEvent, MatrixClientPeg.get());
+    let factory = pickFactory(mxEvent, MatrixClientPeg.get(), showHiddenEvents);
 
     // Info messages are basically information about commands processed on a room
     let isBubbleMessage = (
@@ -92,11 +92,11 @@ export function getEventDisplayInfo(mxEvent: MatrixEvent, hideEvent?: boolean):
     // source tile when there's no regular tile for an event and also for
     // replace relations (which otherwise would display as a confusing
     // duplicate of the thing they are replacing).
-    if (hideEvent || !haveRendererForEvent(mxEvent)) {
+    if (hideEvent || !haveRendererForEvent(mxEvent, showHiddenEvents)) {
         // forcefully ask for a factory for a hidden event (hidden event
         // setting is checked internally)
         // TODO: Thread a MatrixClient through to here
-        factory = pickFactory(mxEvent, MatrixClientPeg.get(), true);
+        factory = pickFactory(mxEvent, MatrixClientPeg.get(), showHiddenEvents, true);
         if (factory === JSONEventFactory) {
             isBubbleMessage = false;
             // Reuse info message avatar and sender profile styling

src/utils/EventUtils.ts

@@ -151,6 +151,16 @@ export enum MessageModerationState {
     SEE_THROUGH_FOR_CURRENT_USER = "SEE_THROUGH_FOR_CURRENT_USER",
 }
 
+// This is lazily initialized and cached since getMessageModerationState needs it,
+// and is called on timeline rendering hot-paths
+let msc3531Enabled: boolean | null = null;
+const getMsc3531Enabled = (): boolean => {
+    if (msc3531Enabled === null) {
+        msc3531Enabled = SettingsStore.getValue("feature_msc3531_hide_messages_pending_moderation");
+    }
+    return msc3531Enabled;
+};
+
 /**
  * Determine whether a message should be displayed as hidden pending moderation.
  *
@@ -160,7 +170,7 @@ export enum MessageModerationState {
 export function getMessageModerationState(mxEvent: MatrixEvent, client?: MatrixClient): MessageModerationState {
     client = client ?? MatrixClientPeg.get(); // because param defaults don't do the correct thing
 
-    if (!SettingsStore.getValue("feature_msc3531_hide_messages_pending_moderation")) {
+    if (!getMsc3531Enabled()) {
         return MessageModerationState.VISIBLE_FOR_ALL;
     }
     const visibility = mxEvent.messageVisibility();

src/utils/exportUtils/HtmlExport.tsx

@@ -407,7 +407,7 @@ export default class HTMLExporter extends Exporter {
                 total: events.length,
             }), false, true);
             if (this.cancelled) return this.cleanUp();
-            if (!haveRendererForEvent(event)) continue;
+            if (!haveRendererForEvent(event, false)) continue;
 
             content += this.needsDateSeparator(event, prevEvent) ? this.getDateSeparator(event) : "";
             const shouldBeJoined = !this.needsDateSeparator(event, prevEvent) &&

src/utils/exportUtils/JSONExport.ts

@@ -85,7 +85,7 @@ export default class JSONExporter extends Exporter {
                 total: events.length,
             }), false, true);
             if (this.cancelled) return this.cleanUp();
-            if (!haveRendererForEvent(event)) continue;
+            if (!haveRendererForEvent(event, false)) continue;
             this.messages.push(await this.getJSONString(event));
         }
         return this.createJSONString();

src/utils/exportUtils/PlainTextExport.ts

@@ -112,7 +112,7 @@ export default class PlainTextExporter extends Exporter {
                 total: events.length,
             }), false, true);
             if (this.cancelled) return this.cleanUp();
-            if (!haveRendererForEvent(event)) continue;
+            if (!haveRendererForEvent(event, false)) continue;
             const textForEvent = await this.plainTextForEvent(event);
             content += textForEvent && `${new Date(event.getTs()).toLocaleString()} - ${textForEvent}\n`;
         }

test/components/structures/MessagePanel-test.js

@@ -276,6 +276,30 @@ describe('MessagePanel', function() {
             }),
         ];
     }
+
+    function mkMixedHiddenAndShownEvents() {
+        const roomId = "!room:id";
+        const userId = "@alice:example.org";
+        const ts0 = Date.now();
+
+        return [
+            TestUtilsMatrix.mkMessage({
+                event: true,
+                room: roomId,
+                user: userId,
+                ts: ts0,
+            }),
+            TestUtilsMatrix.mkEvent({
+                event: true,
+                type: "org.example.a_hidden_event",
+                room: roomId,
+                user: userId,
+                content: {},
+                ts: ts0 + 1,
+            }),
+        ];
+    }
+
     function isReadMarkerVisible(rmContainer) {
         return rmContainer && rmContainer.children.length > 0;
     }
@@ -594,6 +618,21 @@ describe('MessagePanel', function() {
         expect(els.first().prop("events").length).toEqual(5);
         expect(els.last().prop("events").length).toEqual(5);
     });
+
+    // We test this because setting lookups can be *slow*, and we don't want
+    // them to happen in this code path
+    it("doesn't lookup showHiddenEventsInTimeline while rendering", () => {
+        // We're only interested in the setting lookups that happen on every render,
+        // rather than those happening on first mount, so let's get those out of the way
+        const res = mount(<WrappedMessagePanel events={[]} />);
+
+        // Set up our spy and re-render with new events
+        const settingsSpy = jest.spyOn(SettingsStore, "getValue").mockClear();
+        res.setProps({ events: mkMixedHiddenAndShownEvents() });
+
+        expect(settingsSpy).not.toHaveBeenCalledWith("showHiddenEventsInTimeline");
+        settingsSpy.mockRestore();
+    });
 });
 
 describe("shouldFormContinuation", () => {

test/components/views/rooms/MessageComposerButtons-test.tsx

@@ -227,7 +227,7 @@ function createRoomState(room: Room, narrow: boolean): IRoomState {
         showTwelveHourTimestamps: false,
         readMarkerInViewThresholdMs: 3000,
         readMarkerOutOfViewThresholdMs: 30000,
-        showHiddenEventsInTimeline: false,
+        showHiddenEvents: false,
         showReadReceipts: true,
         showRedactions: true,
         showJoinLeaves: true,

test/components/views/rooms/SendMessageComposer-test.tsx

@@ -73,7 +73,7 @@ describe('<SendMessageComposer/>', () => {
         showTwelveHourTimestamps: false,
         readMarkerInViewThresholdMs: 3000,
         readMarkerOutOfViewThresholdMs: 30000,
-        showHiddenEventsInTimeline: false,
+        showHiddenEvents: false,
         showReadReceipts: true,
         showRedactions: true,
         showJoinLeaves: true,