fix: scroll the attachment list instead of burying the send button

Expanding a long attachment list made the composer unusable. With a few dozen files, "+N" grew the chip row past the bottom of the composer: the last rows were off-screen, the send toolbar went with them, and nothing scrolled, so the only way out was to collapse the list again. The row is a shrink-0 sibling of the body's scroll container, so every pixel it takes comes out of the composer and out of nothing else, and it has no scroller of its own. Collapsed that is fine - three chips and a counter. Expanded, its height is the attachment count. So bound it when it is expanded and let it scroll itself. 30vh keeps the toolbar on screen on a laptop and still shows several rows; collapsed behaviour is untouched, since three chips never needed a cap. overscroll-contain stops a scroll that reaches the end of the list from continuing into the message body behind it, which is the usual annoyance once a small scroller appears next to a large one.
2026-11-01 08:12:43 +00:00 · 2026-10-11 18:04:26 +00:00
parent 7aa35ca5b9
commit 58a4041aaa
2 changed files with 286 additions and 1 deletions
@@ -0,0 +1,272 @@
+import { render, screen, fireEvent } from '@testing-library/react';
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import React from 'react';
+import { EmailComposer } from '../email-composer';
+
+// ─── Heavy component mocks (mirrors empty-subject-warning.test.tsx) ────────────
+
+vi.mock('@/components/email/rich-text-editor', () => ({
+  RichTextEditor: () => React.createElement('div', { 'data-testid': 'rich-text-editor' }),
+}));
+
+vi.mock('@/components/plugins/plugin-slot', () => ({ PluginSlot: () => null }));
+vi.mock('@/components/identity/sub-address-helper', () => ({ SubAddressHelper: () => null }));
+vi.mock('@/components/templates/template-picker', () => ({ TemplatePicker: () => null }));
+vi.mock('@/components/templates/template-form', () => ({ TemplateForm: () => null }));
+vi.mock('@/components/files/file-preview-modal', () => ({ FilePreviewModal: () => null }));
+vi.mock('@/hooks/use-focus-trap', () => ({
+  useFocusTrap: () => ({ current: null }),
+}));
+vi.mock('@/hooks/use-pro-multi-account-identities', () => ({
+  useProMultiAccountIdentities: () => ({ enabled: false, groups: [], allIdentities: [] }),
+  stripCrossAccountIdentityPrefix: (id: string) => ({ localAccountId: null, rawId: id }),
+}));
+
+// ─── Store mocks ──────────────────────────────────────────────────────────────
+
+vi.mock('@/stores/auth-store', () => {
+  const state = {
+    client: null,
+    identities: [],
+    primaryIdentity: null,
+    isAuthenticated: false,
+    isDemoMode: false,
+    activeAccountId: null,
+    connectionLost: false,
+    getClientForAccount: () => undefined,
+    getAllConnectedClients: () => new Map(),
+    syncIdentities: () => {},
+    refreshIdentities: async () => {},
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useAuthStore: hook };
+});
+
+vi.mock('@/stores/identity-store', () => {
+  const state = {
+    identities: [{ id: 'id-me', email: 'me@example.com', name: 'Me' }],
+    defaultIdentityId: 'id-me',
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useIdentityStore: hook };
+});
+
+vi.mock('@/stores/account-store', () => {
+  const state = { accounts: [], getAccountById: () => undefined };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useAccountStore: hook };
+});
+
+vi.mock('@/stores/email-store', () => {
+  const state = {
+    draftSaveEnabled: false,
+    sendRawEmail: async () => ({ sent: true }),
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useEmailStore: hook };
+});
+
+const updateSetting = vi.fn();
+
+vi.mock('@/stores/settings-store', () => {
+  const state = {
+    timeFormat: '24h',
+    plainTextMode: false,
+    subAddressDelimiter: '+',
+    autoSelectReplyIdentity: true,
+    attachmentReminderEnabled: false,
+    attachmentReminderKeywords: [],
+    emptySubjectWarningEnabled: true,
+    sendDelaySeconds: 0,
+    signaturePosition: 'above_quote',
+    signatureSeparatorEnabled: false,
+    requestReadReceiptDefault: false,
+    addTrustedSender: () => {},
+    trustedSendersAddressBook: null,
+    updateSetting: (...args: unknown[]) => updateSetting(...args),
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useSettingsStore: hook };
+});
+
+vi.mock('@/stores/contact-store', () => {
+  const state = {
+    contacts: [],
+    getAutocomplete: async () => [],
+    addToTrustedSendersBook: async () => {},
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useContactStore: hook };
+});
+
+vi.mock('@/stores/template-store', () => {
+  const state = { templates: [], addTemplate: async () => {} };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useTemplateStore: hook };
+});
+
+// ─── Misc dependency mocks ────────────────────────────────────────────────────
+
+vi.mock('@/stores/toast-store', () => ({
+  toast: { info: () => {}, error: () => {}, success: () => {} },
+}));
+
+vi.mock('@/lib/plugin-hooks', () => ({
+  emailHooks: {
+    onComposerOpen: { call: async () => [] },
+    onRecipientChange: { call: async () => [] },
+    getRecipientSuggestions: { call: async () => [] },
+    onRecipientChipsChange: { transform: async (chips: unknown) => chips },
+    onDraftChange: { emit: () => {} },
+    onBeforeEmailSend: { intercept: async () => true },
+    onComposeSend: { intercept: async () => true },
+    onTransformOutgoingEmail: { transform: async (email: unknown) => email },
+  },
+  contactHooks: {
+    search: { call: async () => [] },
+    onProvideRecipientSuggestions: { transform: async (initial: unknown) => initial },
+  },
+}));
+
+vi.mock('@/lib/email-sanitization', () => ({
+  sanitizeSignatureHtml: (v: string) => v,
+  sanitizeEmailHtml: (v: string) => v,
+  parseHtmlSafely: (html: string) => new DOMParser().parseFromString(html, 'text/html'),
+}));
+
+vi.mock('@/lib/email-threading', () => ({
+  computeReplyThreadingHeaders: () => ({ inReplyTo: [], references: [] }),
+}));
+vi.mock('@/lib/signature-utils', () => ({
+  appendPlainTextSignature: (body: string) => body,
+  getPlainTextSignature: () => '',
+}));
+vi.mock('@/lib/sub-addressing', () => ({ generateSubAddress: () => '' }));
+vi.mock('@/lib/debug', () => ({ debug: { log: () => {}, warn: () => {}, error: () => {} } }));
+vi.mock('@/components/email/quoted-html', () => ({
+  buildQuotedHtmlBlock: () => '',
+  serializeEditorContent: () => '',
+}));
+vi.mock('@/lib/template-utils', () => ({ substitutePlaceholders: (s: string) => s }));
+
+// ─── Layout stubs ───────────────────────────────────────────────────────────────
+
+// jsdom has no layout and no ResizeObserver. Record what each observer
+// watches so a test can fire the composer's own observer after setting the
+// heights it reads.
+const observers: { callback: ResizeObserverCallback; targets: Element[] }[] = [];
+
+class ResizeObserverStub {
+  private entry: { callback: ResizeObserverCallback; targets: Element[] };
+  constructor(callback: ResizeObserverCallback) {
+    this.entry = { callback, targets: [] };
+    observers.push(this.entry);
+  }
+  observe(target: Element) { this.entry.targets.push(target); }
+  unobserve() {}
+  disconnect() { this.entry.targets = []; }
+}
+
+const ATTACHMENTS = Array.from({ length: 12 }, (_, i) => ({
+  blobId: `blob-${i}`,
+  name: `invoice-${i}.pdf`,
+  type: 'application/pdf',
+  size: 1024 * (i + 1),
+}));
+
+const DRAFT = {
+  to: 'bob@example.com',
+  cc: '',
+  bcc: '',
+  subject: 'Invoices',
+  body: '<p>see attached</p>',
+  showCc: false,
+  showBcc: false,
+  selectedIdentityId: 'id-me',
+  subAddressTag: '',
+  mode: 'compose' as const,
+  draftId: null,
+  attachments: ATTACHMENTS,
+};
+
+/** The wrapping flex row that holds every chip and the expand button. */
+function chipRow() {
+  return screen.getByText('invoice-0.pdf').closest('[class*="flex-wrap"]') as HTMLElement;
+}
+
+/**
+ * The attachment row is a shrink-0 sibling of the body's scroll container,
+ * so whatever height it takes is taken from the composer and from nothing
+ * else. Collapsed that is three chips. Expanded it is one row per two
+ * attachments, and with a few dozen files it grew past the bottom of the
+ * composer: the send toolbar was pushed off-screen and no ancestor
+ * scrolled, so neither the rest of the list nor Send could be reached.
+ * Expanding therefore has to bound the row and scroll it itself.
+ */
+describe('expanded attachment list', () => {
+  beforeEach(() => {
+    observers.length = 0;
+    vi.stubGlobal('ResizeObserver', ResizeObserverStub);
+  });
+
+  afterEach(() => {
+    vi.unstubAllGlobals();
+  });
+
+  it('does not scroll while collapsed', () => {
+    render(<EmailComposer initialData={DRAFT} />);
+
+    expect(chipRow().className).not.toContain('overflow-y-auto');
+  });
+
+  it('shows only the first three attachments and a counter for the rest', () => {
+    render(<EmailComposer initialData={DRAFT} />);
+
+    expect(screen.getByText('invoice-2.pdf')).toBeInTheDocument();
+    expect(screen.queryByText('invoice-3.pdf')).not.toBeInTheDocument();
+    expect(screen.getByText(`+${ATTACHMENTS.length - 3}`)).toBeInTheDocument();
+  });
+
+  it('caps its height and scrolls once expanded', () => {
+    render(<EmailComposer initialData={DRAFT} />);
+
+    fireEvent.click(screen.getByText(`+${ATTACHMENTS.length - 3}`));
+
+    const row = chipRow();
+    expect(screen.getByText('invoice-11.pdf')).toBeInTheDocument();
+    expect(row.className).toContain('overflow-y-auto');
+    // Bounded, or the row grows with the attachment count again and takes
+    // the send toolbar off the bottom of the composer with it.
+    expect(row.className).toContain('max-h-[');
+  });
+
+  it('stops scrolling again when collapsed back', () => {
+    render(<EmailComposer initialData={DRAFT} />);
+
+    fireEvent.click(screen.getByText(`+${ATTACHMENTS.length - 3}`));
+    fireEvent.click(screen.getByText('show_less'));
+
+    expect(chipRow().className).not.toContain('overflow-y-auto');
+  });
+});
@@ -3358,7 +3358,20 @@ export function EmailComposer({
        {/* Attachments */}
        {attachments.length > 0 && (
          <div className="px-4 py-2 border-t shrink-0">
-            <div className="flex flex-wrap gap-2">
+            <div
+              className={cn(
+                "flex flex-wrap gap-2",
+                // Collapsed this is three chips and a counter, so it stays
+                // small. Expanded it is as tall as the attachment count
+                // makes it, and it sits outside the body's scroll container
+                // and cannot shrink - so a few dozen files grew it past the
+                // composer, pushed the send toolbar off the bottom and left
+                // nothing that scrolled: the list was stuck with its last
+                // rows and the Send button both unreachable. Cap the
+                // expanded list and give it a scroller of its own.
+                showAllAttachments && "max-h-[30vh] overflow-y-auto overscroll-contain",
+              )}
+            >
              {(showAllAttachments ? attachments : attachments.slice(0, 3)).map((att, index) => {
                // Clickable to preview only once it has content (local File or an
                // uploaded blob) and the type is previewable; never mid-upload.