fix: Invalid sort comparator for frequently used items (#13258)

* fix: Invalid sort comparator for frequently used items (#13241)

Extracts frequency tracking for icons, emojis, and code languages into a
shared FrequencyTracker with a spec-compliant, stable sort.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Snapshot frequently used icons while picker is open

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Sync frequent custom emojis to tracked ids, preserving order

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

app/components/IconPicker/components/EmojiPanel.tsx

@@ -56,19 +56,14 @@ const EmojiPanel = ({
     emojiSkinTone: skinTone,
     setEmojiSkinTone,
     incrementIconCount,
-    getFrequentIcons,
+    frequentIcons: freqEmojis,
   } = useIconState(IconType.Emoji);
 
   const {
     incrementIconCount: incrementCustomIconCount,
-    getFrequentIcons: getFrequentCustomIcons,
+    frequentIcons: freqCustomEmojiIds,
   } = useIconState(IconType.Custom);
 
-  const freqEmojis = React.useMemo(
-    () => getFrequentIcons(),
-    [getFrequentIcons]
-  );
-
   const [freqCustomEmojis, setFreqCustomEmojis] = React.useState<EmojiNode[]>(
     []
   );
@@ -119,23 +114,16 @@ const EmojiPanel = ({
   );
 
   React.useEffect(() => {
-    // Load frequent custom emojis
-    getFrequentCustomIcons().forEach((id) => {
-      emojis
-        .fetch(id)
-        .then((emoji) => {
-          setFreqCustomEmojis((prev) => {
-            if (prev.some((item) => item.id === id)) {
-              return prev;
-            }
-            return [...prev, toIcon(emoji)];
-          });
-        })
-        .catch(() => {
-          // ignore
-        });
+    // Load frequent custom emojis, preserving frequency order and skipping
+    // any that no longer exist.
+    void Promise.all(
+      freqCustomEmojiIds.map((id) => emojis.fetch(id).catch(() => undefined))
+    ).then((fetched) => {
+      setFreqCustomEmojis(
+        fetched.flatMap((emoji) => (emoji ? [toIcon(emoji)] : []))
+      );
     });
-  }, [emojis, getFrequentCustomIcons]);
+  }, [emojis, freqCustomEmojiIds]);
 
   const [activeEmoji, setActiveEmoji] = React.useState<EmojiNode>();
   const [hasMoreBelow, setHasMoreBelow] = React.useState(false);

app/components/IconPicker/components/IconPanel.tsx

@@ -47,10 +47,9 @@ const IconPanel = ({
   const searchRef = React.useRef<HTMLInputElement | null>(null);
   const scrollableRef = React.useRef<HTMLDivElement | null>(null);
 
-  const { incrementIconCount, getFrequentIcons } = useIconState(IconType.SVG);
+  const { incrementIconCount, frequentIcons } = useIconState(IconType.SVG);
 
-  const freqIcons = React.useMemo(() => getFrequentIcons(), [getFrequentIcons]);
-  const totalFreqIcons = freqIcons.length;
+  const totalFrequentIcons = frequentIcons.length;
 
   const filteredIcons = React.useMemo(
     () => IconLibrary.findIcons(query),
@@ -59,7 +58,7 @@ const IconPanel = ({
 
   const isSearch = query !== "";
   const category = isSearch ? DisplayCategory.Search : DisplayCategory.All;
-  const delayPerIcon = 250 / (TotalIcons + totalFreqIcons);
+  const delayPerIcon = 250 / (TotalIcons + totalFrequentIcons);
 
   const handleFilter = React.useCallback(
     (event: React.ChangeEvent<HTMLInputElement>) => {
@@ -92,7 +91,7 @@ const IconPanel = ({
   // Preview the first icon shown in the grid until the user hovers another.
   const previewIcon =
     activeIcon ??
-    (isSearch ? filteredIcons[0] : (freqIcons[0] ?? filteredIcons[0]));
+    (isSearch ? filteredIcons[0] : (frequentIcons[0] ?? filteredIcons[0]));
 
   const baseIcons: DataNode = {
     category,
@@ -101,7 +100,7 @@ const IconPanel = ({
       name,
       color,
       initial,
-      delay: Math.round((index + totalFreqIcons) * delayPerIcon),
+      delay: Math.round((index + totalFrequentIcons) * delayPerIcon),
       onClick: handleIconSelection,
     })),
   };
@@ -111,12 +110,12 @@ const IconPanel = ({
     : [
         {
           category: DisplayCategory.Frequent,
-          icons: freqIcons.map((name, index) => ({
+          icons: frequentIcons.map((name, index) => ({
             type: IconType.SVG,
             name,
             color,
             initial,
-            delay: Math.round((index + totalFreqIcons) * delayPerIcon),
+            delay: Math.round((index + totalFrequentIcons) * delayPerIcon),
             onClick: handleIconSelection,
           })),
         },

app/components/IconPicker/useIconState.tsx

@@ -1,29 +1,7 @@
-import {
-  customEmojisFreqKey,
-  emojisFreqKey,
-  emojiSkinToneKey,
-  FREQUENTLY_USED_COUNT,
-  iconsFreqKey,
-  lastCustomEmojiKey,
-  lastEmojiKey,
-  lastIconKey,
-  sortFrequencies,
-} from "./utils";
+import { emojiSkinToneKey, iconFrequencies } from "./utils";
+import useFrequencyTracker from "~/hooks/useFrequencyTracker";
 import usePersistedState from "~/hooks/usePersistedState";
 import { EmojiSkinTone, IconType } from "@shared/types";
-import React from "react";
-
-const lastIconKeys = {
-  [IconType.Custom]: lastCustomEmojiKey,
-  [IconType.Emoji]: lastEmojiKey,
-  [IconType.SVG]: lastIconKey,
-};
-
-const freqIconKeys = {
-  [IconType.Custom]: customEmojisFreqKey,
-  [IconType.Emoji]: emojisFreqKey,
-  [IconType.SVG]: iconsFreqKey,
-};
 
 const skinToneKeys = {
   [IconType.Custom]: "",
@@ -37,53 +15,13 @@ export const useIconState = (type: IconType) => {
     EmojiSkinTone.Default
   );
 
-  const [iconFreq, setIconFreq] = usePersistedState<Record<string, number>>(
-    freqIconKeys[type],
-    {}
-  );
-
-  const [lastIcon, setLastIcon] = usePersistedState<string | undefined>(
-    lastIconKeys[type],
-    undefined
-  );
-
-  const incrementIconCount = React.useCallback(
-    (emoji: string) => {
-      iconFreq[emoji] = (iconFreq[emoji] ?? 0) + 1;
-      setIconFreq({ ...iconFreq });
-      setLastIcon(emoji);
-    },
-    [iconFreq, setIconFreq, setLastIcon]
-  );
-
-  const getFrequentIcons = React.useCallback((): string[] => {
-    const freqs = Object.entries(iconFreq);
-
-    if (freqs.length > FREQUENTLY_USED_COUNT.Track) {
-      const trimmed = sortFrequencies(freqs).slice(
-        0,
-        FREQUENTLY_USED_COUNT.Track
-      );
-      setIconFreq(Object.fromEntries(trimmed));
-    }
-
-    const emojis = sortFrequencies(freqs)
-      .slice(0, FREQUENTLY_USED_COUNT.Get)
-      .map(([emoji, _]) => emoji);
-
-    const isLastPresent = emojis.includes(lastIcon ?? "");
-    if (lastIcon && !isLastPresent) {
-      emojis.pop();
-      emojis.push(lastIcon);
-    }
-
-    return emojis;
-  }, [iconFreq, lastIcon, setIconFreq]);
+  const { frequent: frequentIcons, track: incrementIconCount } =
+    useFrequencyTracker(iconFrequencies[type]);
 
   return {
     emojiSkinTone,
     setEmojiSkinTone,
     incrementIconCount,
-    getFrequentIcons,
+    frequentIcons,
   };
 };

app/components/IconPicker/utils.ts

@@ -1,4 +1,6 @@
 import i18next from "i18next";
+import { IconType } from "@shared/types";
+import { FrequencyTracker } from "@shared/utils/FrequencyTracker";
 
 export enum DisplayCategory {
   All = "All",
@@ -21,11 +23,6 @@ export const TRANSLATED_CATEGORIES = {
   Custom: i18next.t("Custom"),
 };
 
-export const FREQUENTLY_USED_COUNT = {
-  Get: 24,
-  Track: 30,
-};
-
 const STORAGE_KEYS = {
   Base: "icon-state",
   EmojiSkinTone: "emoji-skintone",
@@ -41,19 +38,26 @@ const getStorageKey = (key: string) => `${STORAGE_KEYS.Base}.${key}`;
 
 export const emojiSkinToneKey = getStorageKey(STORAGE_KEYS.EmojiSkinTone);
 
-export const iconsFreqKey = getStorageKey(STORAGE_KEYS.IconsFrequency);
+const createFrequencyTracker = (freqKey: string, lastKey: string) =>
+  new FrequencyTracker<string>({
+    key: getStorageKey(freqKey),
+    recentKey: getStorageKey(lastKey),
+    track: 30,
+    get: 24,
+  });
 
-export const emojisFreqKey = getStorageKey(STORAGE_KEYS.EmojisFrequency);
-
-export const lastIconKey = getStorageKey(STORAGE_KEYS.LastIcon);
-
-export const lastEmojiKey = getStorageKey(STORAGE_KEYS.LastEmoji);
-
-export const customEmojisFreqKey = getStorageKey(
-  STORAGE_KEYS.CustomEmojisFrequency
-);
-
-export const lastCustomEmojiKey = getStorageKey(STORAGE_KEYS.LastCustomEmoji);
-
-export const sortFrequencies = (freqs: [string, number][]) =>
-  freqs.sort((a, b) => (a[1] >= b[1] ? -1 : 1));
+/** Tracks the icons used most frequently, by type of icon. */
+export const iconFrequencies: Record<IconType, FrequencyTracker<string>> = {
+  [IconType.SVG]: createFrequencyTracker(
+    STORAGE_KEYS.IconsFrequency,
+    STORAGE_KEYS.LastIcon
+  ),
+  [IconType.Emoji]: createFrequencyTracker(
+    STORAGE_KEYS.EmojisFrequency,
+    STORAGE_KEYS.LastEmoji
+  ),
+  [IconType.Custom]: createFrequencyTracker(
+    STORAGE_KEYS.CustomEmojisFrequency,
+    STORAGE_KEYS.LastCustomEmoji
+  ),
+};

app/hooks/useFrequencyTracker.ts

@@ -0,0 +1,18 @@
+import { useCallback, useState } from "react";
+import type { FrequencyTracker } from "@shared/utils/FrequencyTracker";
+
+/**
+ * Read the most frequent items from a tracker, snapshotted on mount so the
+ * order does not shift beneath the user while the component remains open.
+ *
+ * @param tracker the tracker to read from.
+ * @returns the most frequent items and a function to track a use of an item.
+ */
+export default function useFrequencyTracker<T extends string>(
+  tracker: FrequencyTracker<T>
+) {
+  const [frequent] = useState(() => tracker.frequent);
+  const track = useCallback((item: T) => tracker.track(item), [tracker]);
+
+  return { frequent, track };
+}

shared/editor/lib/code.ts

@@ -1,10 +1,5 @@
 import type { RefractorSyntax } from "refractor";
-import Storage from "../../utils/Storage";
-
-const RecentlyUsedStorageKey = "rme-code-language";
-const StorageKey = "frequent-code-languages";
-const frequentLanguagesToGet = 5;
-const frequentLanguagesToTrack = 10;
+import { FrequencyTracker } from "../../utils/FrequencyTracker";
 
 /**
  * Describes a code language supported by the editor.
@@ -394,89 +389,35 @@ const nonPersistableLanguages = ["mermaid", "mermaidjs"];
 const isPersistableCodeLanguage = (language: string) =>
   !nonPersistableLanguages.includes(language);
 
+const codeLanguageFrequency = new FrequencyTracker<keyof typeof codeLanguages>({
+  key: "frequent-code-languages",
+  recentKey: "rme-code-language",
+  track: 10,
+  get: 5,
+  filter: isPersistableCodeLanguage,
+});
+
 /**
  * Set the most recent code language used.
  *
  * @param language The language identifier.
  */
-export const setRecentlyUsedCodeLanguage = (language: string) => {
-  if (!isPersistableCodeLanguage(language)) {
-    return;
-  }
-
-  const frequentLangs = (Storage.get(StorageKey) ?? {}) as Record<
-    string,
-    number
-  >;
-
-  if (Object.keys(frequentLangs).length === 0) {
-    const lastUsedLang = Storage.get(RecentlyUsedStorageKey);
-    if (lastUsedLang) {
-      frequentLangs[lastUsedLang] = 1;
-    }
-  }
-
-  frequentLangs[language] = (frequentLangs[language] ?? 0) + 1;
-
-  const frequentLangEntries = Object.entries(frequentLangs);
-
-  if (frequentLangEntries.length > frequentLanguagesToTrack) {
-    sortFrequencies(frequentLangEntries);
-
-    const lastEntry = frequentLangEntries[frequentLanguagesToTrack];
-    if (lastEntry[0] === language) {
-      frequentLangEntries.splice(frequentLanguagesToTrack - 1, 1);
-    } else {
-      frequentLangEntries.splice(frequentLanguagesToTrack);
-    }
-  }
-
-  Storage.set(StorageKey, Object.fromEntries(frequentLangEntries));
-  Storage.set(RecentlyUsedStorageKey, language);
-};
+export const setRecentlyUsedCodeLanguage = (language: string) =>
+  codeLanguageFrequency.track(language as keyof typeof codeLanguages);
 
 /**
  * Get the most recent code language used.
  *
  * @returns The most recent code language used, or undefined if none is set.
  */
-export const getRecentlyUsedCodeLanguage = () => {
-  const language = Storage.get(RecentlyUsedStorageKey) as
-    | keyof typeof codeLanguages
-    | undefined;
-  return language && isPersistableCodeLanguage(language) ? language : undefined;
-};
+export const getRecentlyUsedCodeLanguage = () => codeLanguageFrequency.recent;
 
 /**
  * Get the most frequent code languages used.
  *
  * @returns An array of the most frequent code languages used.
  */
-export const getFrequentCodeLanguages = () => {
-  const recentLang = getRecentlyUsedCodeLanguage();
-  const frequentLangEntries = (
-    Object.entries(Storage.get(StorageKey) ?? {}) as [
-      keyof typeof codeLanguages,
-      number,
-    ][]
-  ).filter(([lang]) => isPersistableCodeLanguage(lang));
-
-  const frequentLangs = sortFrequencies(frequentLangEntries)
-    .slice(0, frequentLanguagesToGet)
-    .map(([lang]) => lang);
-
-  const isRecentLangPresent =
-    !!recentLang && frequentLangs.includes(recentLang);
-  if (recentLang && !isRecentLangPresent) {
-    frequentLangs.pop();
-    frequentLangs.push(recentLang);
-  }
-
-  return frequentLangs;
-};
-
-const sortFrequencies = <T>(freqs: [T, number][]) =>
-  freqs.sort((a, b) => (a[1] >= b[1] ? -1 : 1));
+export const getFrequentCodeLanguages = () => codeLanguageFrequency.frequent;
 
 export const languagesWithFourSpaceIndent = [
   "python",

shared/utils/FrequencyTracker.test.ts

@@ -0,0 +1,91 @@
+import { FrequencyTracker } from "./FrequencyTracker";
+import Storage from "./Storage";
+
+const createTracker = (options?: { filter?: (item: string) => boolean }) =>
+  new FrequencyTracker<string>({
+    key: "test-freq",
+    recentKey: "test-recent",
+    track: 3,
+    get: 2,
+    ...options,
+  });
+
+describe("FrequencyTracker", () => {
+  beforeEach(() => {
+    Storage.clear();
+  });
+
+  it("returns the most recently tracked item", () => {
+    const tracker = createTracker();
+    tracker.track("a");
+    tracker.track("b");
+    expect(tracker.recent).toBe("b");
+  });
+
+  it("returns nothing when nothing has been tracked", () => {
+    expect(createTracker().recent).toBeUndefined();
+    expect(createTracker().frequent).toEqual([]);
+  });
+
+  it("orders items by frequency, most frequent first", () => {
+    const tracker = createTracker();
+    tracker.track("a");
+    tracker.track("b");
+    tracker.track("b");
+    expect(tracker.frequent).toEqual(["b", "a"]);
+  });
+
+  it("preserves insertion order for items of equal frequency", () => {
+    const tracker = createTracker();
+    tracker.track("a");
+    tracker.track("b");
+    expect(tracker.frequent).toEqual(["a", "b"]);
+  });
+
+  it("limits the items returned, always including the recent one", () => {
+    const tracker = createTracker();
+    tracker.track("a");
+    tracker.track("a");
+    tracker.track("b");
+    tracker.track("b");
+    tracker.track("c");
+    expect(tracker.frequent).toEqual(["a", "c"]);
+  });
+
+  it("does not drop an item when the recent one already fits", () => {
+    const tracker = createTracker();
+    tracker.track("a");
+    tracker.track("a");
+    tracker.track("b");
+    expect(tracker.frequent).toEqual(["a", "b"]);
+  });
+
+  it("keeps counts for a limited number of items", () => {
+    const tracker = createTracker();
+    ["a", "a", "b", "b", "c", "c", "d"].forEach((item) => tracker.track(item));
+    expect(Storage.get("test-freq")).toEqual({ a: 2, b: 2, d: 1 });
+  });
+
+  it("ignores items excluded by the filter", () => {
+    const tracker = createTracker({ filter: (item) => item !== "b" });
+    tracker.track("a");
+    tracker.track("b");
+    expect(tracker.recent).toBe("a");
+    expect(tracker.frequent).toEqual(["a"]);
+  });
+
+  it("excludes filtered items that were previously persisted", () => {
+    const tracker = createTracker({ filter: (item) => item !== "b" });
+    Storage.set("test-freq", { a: 1, b: 5 });
+    Storage.set("test-recent", "b");
+    expect(tracker.recent).toBeUndefined();
+    expect(tracker.frequent).toEqual(["a"]);
+  });
+
+  it("seeds counts from the recent item when there are none", () => {
+    const tracker = createTracker();
+    Storage.set("test-recent", "a");
+    tracker.track("b");
+    expect(tracker.frequent).toEqual(["a", "b"]);
+  });
+});

shared/utils/FrequencyTracker.ts

@@ -0,0 +1,106 @@
+import Storage from "./Storage";
+
+export interface FrequencyTrackerOptions<T extends string> {
+  /** Storage key under which the counts are persisted. */
+  key: string;
+  /** Storage key under which the most recently tracked item is persisted. */
+  recentKey: string;
+  /** Maximum number of items to keep counts for. */
+  track: number;
+  /** Maximum number of items returned by `frequent`. */
+  get: number;
+  /** Optional predicate deciding which items may be tracked and returned. */
+  filter?: (item: T) => boolean;
+}
+
+/**
+ * Tracks how often items are used, persisting the counts locally so the most
+ * frequently and most recently used can be surfaced later.
+ */
+export class FrequencyTracker<T extends string> {
+  public constructor(options: FrequencyTrackerOptions<T>) {
+    this.options = options;
+  }
+
+  /**
+   * The most recently tracked item, or undefined if there is none.
+   */
+  public get recent(): T | undefined {
+    const item = Storage.get(this.options.recentKey) as T | undefined;
+    return item && this.isTrackable(item) ? item : undefined;
+  }
+
+  /**
+   * The most frequently tracked items, most frequent first. The most recently
+   * tracked item is always included.
+   */
+  public get frequent(): T[] {
+    const items = this.sorted(this.counts).slice(0, this.options.get);
+
+    const { recent } = this;
+    if (recent && !items.includes(recent)) {
+      if (items.length === this.options.get) {
+        items.pop();
+      }
+      items.push(recent);
+    }
+
+    return items;
+  }
+
+  /**
+   * Records a use of the given item, making it the most recent.
+   *
+   * @param item the item that was used.
+   */
+  public track(item: T) {
+    if (!this.isTrackable(item)) {
+      return;
+    }
+
+    const counts = this.counts;
+
+    // Seed from the recent item so history isn't lost for those who used this
+    // tracker before counts were persisted.
+    if (Object.keys(counts).length === 0 && this.recent) {
+      counts[this.recent] = 1;
+    }
+
+    counts[item] = (counts[item] ?? 0) + 1;
+
+    const entries = Object.entries(counts) as [T, number][];
+    if (entries.length > this.options.track) {
+      this.sortEntries(entries);
+
+      // Keep the item just tracked, evicting the next least frequent instead.
+      if (entries[this.options.track][0] === item) {
+        entries.splice(this.options.track - 1, 1);
+      }
+      entries.splice(this.options.track);
+    }
+
+    Storage.set(this.options.key, Object.fromEntries(entries));
+    Storage.set(this.options.recentKey, item);
+  }
+
+  private options: FrequencyTrackerOptions<T>;
+
+  private get counts(): Record<T, number> {
+    return (Storage.get(this.options.key) ?? {}) as Record<T, number>;
+  }
+
+  private isTrackable(item: T) {
+    return this.options.filter?.(item) ?? true;
+  }
+
+  private sorted(counts: Record<T, number>): T[] {
+    const entries = (Object.entries(counts) as [T, number][]).filter(([item]) =>
+      this.isTrackable(item)
+    );
+    return this.sortEntries(entries).map(([item]) => item);
+  }
+
+  private sortEntries(entries: [T, number][]) {
+    return entries.sort((a, b) => b[1] - a[1]);
+  }
+}