From 58a4041aaa16e50a77b8316d93b3aa6868237da1 Mon Sep 17 00:00:00 2001 From: Shuki Vaknin Date: Sun, 11 Oct 2026 16:59:43 +0000 Subject: [PATCH] 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. --- .../composer-attachment-list-scroll.test.tsx | 272 ++++++++++++++++++ components/email/email-composer.tsx | 15 +- 2 files changed, 286 insertions(+), 1 deletion(-) create mode 100644 components/email/__tests__/composer-attachment-list-scroll.test.tsx diff --git a/components/email/__tests__/composer-attachment-list-scroll.test.tsx b/components/email/__tests__/composer-attachment-list-scroll.test.tsx new file mode 100644 index 000000000..9efbceeb6 --- /dev/null +++ b/components/email/__tests__/composer-attachment-list-scroll.test.tsx @@ -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) => 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) => 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) => 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) => 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) => 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) => 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) => 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: '

see attached

', + 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(); + + expect(chipRow().className).not.toContain('overflow-y-auto'); + }); + + it('shows only the first three attachments and a counter for the rest', () => { + render(); + + 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(); + + 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(); + + fireEvent.click(screen.getByText(`+${ATTACHMENTS.length - 3}`)); + fireEvent.click(screen.getByText('show_less')); + + expect(chipRow().className).not.toContain('overflow-y-auto'); + }); +}); diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index 509b6e5bc..be13e1d91 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -3358,7 +3358,20 @@ export function EmailComposer({ {/* Attachments */} {attachments.length > 0 && (
-
+
{(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.