Transient dropdown menus stop working until a page refresh (orphaned outside-click listeners)

[Mocchibird]

Prerequisites

• I searched open issues and discussions and did not find an existing report of this bug.
• This is not a security vulnerability.
• I am running the latest code from the dev branch and the bug still reproduces there.

Install Method

Docker (docker compose up)

Operating System

Linux

Steps to Reproduce

1. Open the email library and open an email so the reader view shows.
2. Go back to the list, then permanently delete that email (not trash).
3. Click the "..." (more / kebab) button on any other email row.
4. Observe: the dropdown does not open — the button is dead until the page is reloaded.

This is one instance of a general pattern. The same stale-listener breakage can be triggered on most of the app's ad-hoc dropdown/context menus (gallery bulk menu, notes corner menu, the document export menu, the skills card menu, the tasks dropdown, the settings integration dropdowns, and more) any time a menu is dismissed by something other than an outside click.

Expected Behaviour

After a menu closes by any means — clicking an item, a Cancel button, Escape, or opening a different menu — every other menu trigger keeps working normally. No page refresh should ever be required to "unstick" a menu button.

Actual Behaviour

Each of these menus wires its own document-level outside-click listener, roughly:

const close = (ev) => { if (menu.contains(ev.target)) return; menu.remove(); document.removeEventListener('click', close, true); };
setTimeout(() => document.addEventListener('click', close, true), 10);

That listener only removes itself on an outside click. Every other dismissal path — an item click that calls menu.remove() directly, a Cancel button, Escape, or the "close the previously-open menu" reopen sweep — tears the node down without unregistering the listener, leaving it orphaned on document. The stale listener then interferes with the next menu interaction, so the trigger appears dead until a reload. No console error is produced, which is why it is easy to miss.

The repo already ships the right tool for this: the escMenuStack helper (bindMenuDismiss / dismissOrRemove) introduced in #684 and already used correctly by documentLibrary.js _showLibDropdown — but roughly 22 other menus were never migrated onto it.

Logs / Screenshots

No console error or stack trace is produced; the failure is silent — the menu trigger simply does nothing on the next click until the page is reloaded.

Model / Backend (if relevant)

N/A — this is a front-end (browser) UI bug, independent of the model backend or OS.

Are you willing to submit a fix?

Yes — I can open a PR

Additional Information

Root cause: the orphaned document click listener. Fix: route these menus through the existing escMenuStack helper so a single idempotent close() owns teardown on every dismissal path (item click, Cancel, Escape, outside click, reopen sweep).

A draft PR for the 16 mechanical (clean 1:1 migration) menus is up at #4684, left as a draft pending end-to-end verification. Six menus with more involved lifecycles (the notes reminder menu, the sessions long-press menu, the document markdown-toolbar menu, the emoji picker, and the compare model-selector) are noted there as a follow-up.

Transparency: this bug and the proposed fix were prepared with the assistance of an LLM agent (Claude Code). Per CONTRIBUTING.md's "Auto-generated PRs" note I am filing this issue first and leaving the PR as a draft for your call on whether to proceed.

[entitycs]

Extracted the below from chatRenderer.js, as the pattern was used multiple times in that file alone.

It's still incomplete - but could be part of the solution for this down the road. The problem will be getting all of the spread out occurrences of these almost-equal popup patterns updated.

edit: I see you've submitted the PR, nice... Has that given you any idea of how aligned or not this in-progress forward-looking potential solution is/was? I still plan on keeping it around locally, at least until I give up on reducing chatRenderer.

It assures bindMenuDismiss is known to the element as it is built. It also removes the "left = 8" repeats + almost-repeats all over the frontend code. It binds programmatic .remove() calls to its own .remove() function, so cleanup can be consolidated.

Usage

      openPopover(roleEl, html, {
        placement: 'top',
        align: 'left',
        offset: 8,
        className: 'ctx-popup',
      });

Implementation

expand

/**
 * popups/popover.js
 *
 * Shared popover positioning + single-open registry.
 *
 * Every popup in chatRenderer.js follows the same pattern:
 *   1. Create a positioned <div> with class `.ctx-popup` (or similar).
 *   2. Append it to `document.body` with `visibility: hidden`.
 *   3. Measure its bounding rect.
 *   4. Pick a preferred placement (typically above the trigger), flip if
 *      it would overflow the viewport.
 *   5. Clamp to right/left edges.
 *   6. Set `visibility: ''` to reveal.
 *   7. Call `bindMenuDismiss` so that a click outside or Escape closes it.
 *
 * This module consolidates steps 2–6 into `openPopover()` and adds a
 * **single-open registry**: only one popover (per optional group) can be
 * open at a time. Opening a new one auto-dismisses the previous.
 *
 * Usage (raw DOM — as chatRenderer uses today):
 *
 *   const popup = openPopover(triggerEl, buildContent(), {
 *     group: 'ctx-popup',           // optional; defaults to '__global'
 *     placement: 'top',             // 'top' | 'bottom' | 'auto'
 *     align: 'left',                // 'left' | 'right' | 'auto'
 *     offset: 8,                   // gap between trigger and popover (px)
 *     className: 'ctx-popup',       // CSS class on the popover element
 *     isOutsideClick: (ev) => ..., // optional custom "outside" predicate
 *   });
 *
 *   // popup is the <div> element, already in the DOM.
 *   // It is auto-removed when dismissed.
 *
 * Usage from a React/Svelte port:
 *
 *   Replace this module with a single <Popover> component that wraps the
 *   trigger and content, using a Svelte `popoverStore` or React context
 *   to enforce single-open.  The positioning math is identical.
 */

import { bindMenuDismiss } from '../escMenuStack.js';

/* -- Single-open registry -------------------------------------------------- */

/** @type {Map<string, HTMLElement>} */
const _openPopovers = new Map();

/** @type {Map<HTMLElement, string>} */
const _groupOf = new Map();

/**
 * Dismiss all open popovers in the given group, or all groups if omitted.
 * Each popover is dismissed through its own `close` callback (registered
 * by `bindMenuDismiss`), so any Escape-key listener is cleaned up too.
 *
 * @param {string} [group] - Group key to dismiss. Omit for all.
 */
export function dismissGroup(group) {
  if (group) {
    const el = _openPopovers.get(group);
    if (el && typeof el._dismiss === 'function') el._dismiss();
  } else {
    for (const [, el] of _openPopovers) {
      if (typeof el._dismiss === 'function') el._dismiss();
    }
  }
}

/**
 * Remove a popover from the registry (called after it is removed from DOM).
 * @param {HTMLElement} el
 */
function _unregister(el) {
  const group = _groupOf.get(el);
  if (group) {
    const current = _openPopovers.get(group);
    if (current === el) _openPopovers.delete(group);
  }
  _groupOf.delete(el);
}

/* ── Positioning ──────────────────────────────────────────────────── */

/**
 * Position a popover relative to a trigger element.
 *
 * Strategy:
 *  - Render hidden first to measure.
 *  - Place at preferred side (default: above trigger).
 *  - If it overflows, flip to the opposite side.
 *  - Clamp horizontal position to keep within viewport edges.
 *
 * @param {HTMLElement} popover  - The popup element (already in the DOM).
 * @param {HTMLElement} trigger  - The element that triggered it.
 * @param {object} options
 * @param {'top'|'bottom'|'auto'} [options.placement='top'] - Preferred vertical.
 * @param {'left'|'right'|'auto'} [options.align='left'] - Preferred horizontal.
 * @param {number} [options.offset=8] - Gap (px) between trigger and popover.
 */
export function positionPopover(popover, trigger, options = {}) {
  const { placement = 'top', align = 'left', offset = 8 } = options;
  const pr = popover.getBoundingClientRect();
  const tr = trigger.getBoundingClientRect();
  const gap = offset;

  // Vertical placement
  let top;
  if (placement === 'bottom') {
    top = tr.bottom + gap;
  } else if (placement === 'top') {
    top = tr.top - pr.height - gap;
  } else {
    // auto: prefer above
    const spaceAbove = tr.top;
    const spaceBelow = window.innerHeight - tr.bottom;
    if (spaceAbove >= pr.height + gap || spaceAbove > spaceBelow) {
      top = tr.top - pr.height - gap;
    } else {
      top = tr.bottom + gap;
    }
  }
  // Guard against going above viewport for forced 'top'
  if (top < gap && placement === 'top') {
    top = tr.bottom + gap;
  }

  // Horizontal alignment
  let left;
  if (align === 'right') {
    left = Math.max(gap, tr.right - pr.width);
  } else if (align === 'left') {
    left = tr.left;
  } else {
    // auto: left-align, but clamp right
    left = tr.left;
  }
  // Clamp to viewport right edge
  if (left + pr.width > window.innerWidth - gap) {
    left = window.innerWidth - pr.width - gap;
  }
  // Clamp to viewport left edge
  if (left < gap) left = gap;

  popover.style.top = top + 'px';
  popover.style.left = left + 'px';
}

/* ── Factory ───────────────────────────────────────────────────────── */

/**
 * Create, position, and show a popover.
 *
 * @param {HTMLElement} triggerEl  - Element the popover anchors to.
 * @param {string|HTMLElement} content  - innerHTML string or pre-built DOM node.
 * @param {object} [options]
 * @param {string}  [options.group='__global']  - Single-open group key.
 * @param {'top'|'bottom'|'auto'} [options.placement='top']
 * @param {'left'|'right'|'auto'} [options.align='left']
 * @param {number}  [options.offset=8]
 * @param {string}  [options.className=''] - CSS class(es) for the popover.
 * @param {function(Event): boolean} [options.isOutsideClick] - Custom
 *   predicate that returns `true` when the click target counts as "outside".
 *   When omitted the default treats only the popover body as "inside".
 * @returns {HTMLElement} The popover <div> (already in the DOM).
 */
export function openPopover(triggerEl, content, options = {}) {
  const {
    group = '__global',
    placement = 'top',
    align = 'left',
    offset = 8,
    className = '',
    isOutsideClick,
  } = options;

  // Auto-dismiss previous popover in the same group
  dismissGroup(group);

  // Build element
  const popover = document.createElement('div');
  if (className) popover.className = className;

  if (typeof content === 'string') {
    popover.innerHTML = content;
  } else {
    popover.appendChild(content);
  }

  // Hide, append to measure
  popover.style.visibility = 'hidden';
  document.body.appendChild(popover);

  // Position
  positionPopover(popover, triggerEl, { placement, align, offset });

  // Reveal
  popover.style.visibility = '';

  // Register for single-open tracking
  _openPopovers.set(group, popover);
  _groupOf.set(popover, group);

  // Wire dismiss (Escape + outside click)
  const onClose = () => {
    _unregister(popover);
  };

  const defaultOutside = isOutsideClick
    ? (ev) => !popover.contains(ev.target) && isOutsideClick(ev)
    : undefined;

  bindMenuDismiss(popover, () => popover.remove(), defaultOutside);

  // Also unregister on programmatic remove (e.g. compact calls popup.remove())
  const origRemove = popover.remove.bind(popover);
  popover.remove = function () {
    onClose();
    origRemove();
  };

  return popover;
}

I've been working on trying to get chatRenderer.js down to < 1K lines. In its current state in dev, if you lint it to make it readable, it expands out to 3144 lines. My linted version is down to 2400. So, about ~700 well-linted lines have been extracted out so far, mostly into the static/js/models folder. You can follow the PR starting with refactor(chatRenderer) if interested in that progress.

Beyond the model info and this popover generalization, I'm attempting to remove from it anything that

1. isn't related to rendering chats, or
2. is used / useful outside of the file, and hits that 'component of' vs. 'part of' division.

This 'popover' pattern is a 'component of' numerous elements throughout the app, as you've noted.

The refactor of chatRenderer is also a test of the level of refactoring that would be necessary to align the frontend with a React or SvelteKit.