diff --git a/sidecar/mail-scope.test.ts b/sidecar/mail-scope.test.ts new file mode 100644 index 0000000..e73e8a5 --- /dev/null +++ b/sidecar/mail-scope.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from 'bun:test'; +import { queryAcross } from './mail-scope'; + +// The merge is the part that can be silently wrong: a page with a hole in it looks like missing mail rather +// than like a bug. These pin the property that makes it exact — each mailbox is asked for offset+limit. +const mailbox = (dates: string[]) => + ({ + account: { id: dates.length, email: `a${dates.length}@x` }, + db: {} as never, + }) as never; + +describe('queryAcross', () => { + const A = ['2026-08-10', '2026-08-08', '2026-08-02']; + const B = ['2026-08-09', '2026-08-07', '2026-08-01']; + + const opened = [ + { account: { id: 1, email: 'a@x' }, db: 'A' }, + { account: { id: 2, email: 'b@x' }, db: 'B' }, + ] as never as Parameters[0]; + + const run = (db: unknown, take: number) => + ((db as unknown as string) === 'A' ? A : B).slice(0, take).map((date) => ({ date })); + + it('interleaves newest-first across mailboxes', () => { + const rows = queryAcross(opened, run as never, { sortBy: (r) => String(r.date), limit: 4, offset: 0 }); + expect(rows.map((r) => r.date)).toEqual(['2026-08-10', '2026-08-09', '2026-08-08', '2026-08-07']); + }); + + it('labels every row with the mailbox it came from', () => { + const rows = queryAcross(opened, run as never, { sortBy: (r) => String(r.date), limit: 2, offset: 0 }); + expect(rows.map((r) => r.accountEmail)).toEqual(['a@x', 'b@x']); + }); + + // The reason each mailbox is asked for offset+limit rather than limit: page 2 must not skip a row that + // ranked low locally and high globally. + it('pages without a hole', () => { + const p1 = queryAcross(opened, run as never, { sortBy: (r) => String(r.date), limit: 2, offset: 0 }); + const p2 = queryAcross(opened, run as never, { sortBy: (r) => String(r.date), limit: 2, offset: 2 }); + const p3 = queryAcross(opened, run as never, { sortBy: (r) => String(r.date), limit: 2, offset: 4 }); + expect([...p1, ...p2, ...p3].map((r) => r.date)).toEqual([...A, ...B].sort().reverse()); + }); + + // The contract, pinned: local order must match merge order. A mailbox handing back rows in the wrong + // direction breaks top-K silently, which is why the `descending` option was removed rather than fixed — + // it flipped the merge while every caller's SQL still said ORDER BY date DESC. + it('trusts each mailbox to be newest-first', () => { + const backwards = (db: unknown, take: number) => + ((db as unknown as string) === 'A' ? [...A].reverse() : [...B].reverse()) + .slice(0, take) + .map((date) => ({ date })); + const rows = queryAcross(opened, backwards as never, { sortBy: (r) => String(r.date), limit: 2, offset: 0 }); + // The true two newest are 08-10 and 08-09. Fed oldest-first mailboxes, the helper sorts correctly + // within what it was handed and returns neither — no error, no gap, just quietly the wrong mail. That + // is the failure the written contract exists to prevent, and it is why the `descending` option went. + expect(rows.map((r) => r.date)).toEqual(['2026-08-08', '2026-08-07']); + expect(rows.map((r) => r.date)).not.toContain('2026-08-10'); + }); +}); diff --git a/sidecar/mail-scope.ts b/sidecar/mail-scope.ts new file mode 100644 index 0000000..c3e0496 --- /dev/null +++ b/sidecar/mail-scope.ts @@ -0,0 +1,138 @@ +import type { Database } from 'bun:sqlite'; +import type { EmailAccount } from '../db/types'; +import { getEmailAccounts } from '../db/queries'; +import { openEmailDb } from './store'; + +/* + Which mailboxes a request is about. + + Every read route used to answer this the same way, in a line that appeared twice and was copied a third + time into the web list: + + const account = accounts.find((a) => a.enabled) ?? accounts[0]; + + So an account could be added, could sync, could fill its own database — and could never be read. Thirteen + routes looked at the first one and nothing said so. + + ── One database per account, and no ATTACH ── + + Each account keeps its own `emails.db`. A unified view is therefore a MERGE, not a join, and that is a + choice rather than a limitation: `emails_fts` is a per-database FTS5 virtual table, and `MATCH` cannot be + aimed through a view over an ATTACHed sibling. Merging keeps every SQL string in `store.ts` exactly as it + is — the same query, run per account, combined here. + + The merge is a distributed top-K, which is EXACT rather than approximate: to serve rows + [offset, offset+limit) sorted by date across N mailboxes, ask each for its first (offset+limit) rows and + take the window from the combined sort. Nothing in a mailbox's tail can outrank a row it has already been + outranked by locally. +*/ + +export type MailScope = { + /** The accounts in play. One for a normal request, all enabled ones for `?account=all`. */ + accounts: EmailAccount[]; + /** True when this is the unified view — routes that cannot merge use it to refuse clearly. */ + unified: boolean; +}; + +/** + * Resolve the `account` query parameter. + * + * Absent means "the first enabled one", which is what every existing client sends and what the mobile app + * and the agent's `email_db` tool still assume. Making it required would have broken both silently, so the + * old behaviour is the default and the parameter is how you opt into knowing better. + * + * A numeric id must belong to the caller. `getEmailAccounts(userId)` is already scoped to them, so the id + * is checked against that list rather than fetched directly — an account id from another user resolves to + * nothing rather than to somebody else's mail. + */ +export async function resolveMailScope(userId: number, param: string | undefined): Promise { + const accounts = await getEmailAccounts(userId); + const enabled = accounts.filter((a) => a.enabled); + + if (param === 'all') return { accounts: enabled.length ? enabled : accounts, unified: true }; + + if (param) { + const id = Number(param); + const one = Number.isFinite(id) ? accounts.find((a) => a.id === id) : undefined; + return { accounts: one ? [one] : [], unified: false }; + } + + const first = enabled[0] ?? accounts[0]; + return { accounts: first ? [first] : [], unified: false }; +} + +/** Open every mailbox in the scope, tagged so a merged row can say where it came from. */ +export function openScope(ownerEmail: string, scope: MailScope): Array<{ account: EmailAccount; db: Database }> { + return scope.accounts.map((account) => ({ account, db: openEmailDb(ownerEmail, account.email) })); +} + +type AcrossOptions = { + /** The value each row is ordered by. Dates are ISO strings here, so string compare is chronological. */ + sortBy: (row: T) => string | number; + limit: number; + offset: number; +}; + +/** + * Run one query across several mailboxes and return the correct window of the combined result. + * + * Each mailbox is asked for `offset + limit` rows, not `limit`: a row at position 3 in one mailbox can + * belong at position 40 of the merge, so anything less risks a hole in a later page. That is also the cost + * — page 20 of a unified inbox reads 20 pages from every account. Acceptable for the page depths anybody + * scrolls to by hand, and the reason a unified view should not be the place anyone paginates deeply. + * + * **The per-mailbox query must already be ordered newest-first**, the same direction as the merge. That is + * the whole basis of a top-K merge: taking the first K locally is only safe if local order agrees with + * global order. This briefly had a `descending: false` option, which was wrong in a way a test caught — it + * flipped the merge while the callers' SQL still said `ORDER BY date DESC`, so asking for the oldest mail + * returned the oldest of the newest page. There is no caller for ascending, and an option that is only + * correct if you also remember to flip the SQL is worse than no option. + */ +export function queryAcross>( + opened: Array<{ account: EmailAccount; db: Database }>, + run: (db: Database, take: number) => T[], + options: AcrossOptions, +): Array { + const take = options.offset + options.limit; + const rows = opened.flatMap(({ account, db }) => + run(db, take).map((row) => ({ ...row, accountId: account.id, accountEmail: account.email })), + ); + rows.sort((a, b) => { + const av = options.sortBy(a); + const bv = options.sortBy(b); + return av === bv ? 0 : av > bv ? -1 : 1; + }); + return rows.slice(options.offset, options.offset + options.limit); +} + +/** + * Open the one mailbox holding a message or a thread, closing the rest. + * + * Matches `id` OR `thread_id`, because these routes are addressed by both and the two id spaces overlap by + * design: a thread id is `sha1(Message-Id)` of the root message, so it usually IS a message id — but not + * for mail threaded by the subject fallback, where it is not. One predicate covers both without the caller + * having to know which kind of id it is holding. + * + * Asks each mailbox in turn and stops at the first hit. A collision across accounts would need the same + * message in both, in which case either copy answers the question correctly. + */ +export async function openHolding( + ownerEmail: string, + userId: number, + accountParam: string | undefined, + id: string, +): Promise { + const scope = await resolveMailScope(userId, accountParam); + const opened = openScope(ownerEmail, scope); + let held: Database | null = null; + for (const { db } of opened) { + if (held) { + db.close(); + continue; + } + const hit = db.query('SELECT 1 FROM emails WHERE id = ? OR thread_id = ? LIMIT 1').get(id, id); + if (hit) held = db; + else db.close(); + } + return held; +} diff --git a/sidecar/routes.ts b/sidecar/routes.ts index b7c3110..fc23484 100644 --- a/sidecar/routes.ts +++ b/sidecar/routes.ts @@ -8,18 +8,39 @@ import { getEmailAttachmentCacheDir } from '@@/data-path'; import { getEmailAccounts } from '../db/queries'; import { openEmailDb, openUserEmailDb, rowToSummary, getSyncMeta, searchEmails } from './store'; import { accountsRouter } from './accounts'; +import { openHolding, openScope, queryAcross, resolveMailScope } from './mail-scope'; export const emailRouter = createRouter(); emailRouter.route('/accounts', accountsRouter); // ── Sending (SMTP) — sends as the connected account using its app password ── +/** + * The mailbox a message goes out from. + * + * `accountParam` is the account id the composer names. It matters most for a REPLY: mail arrives on one + * account, and answering it from another is not a cosmetic slip — it leaks which addresses belong to the + * same person, and the thread breaks for the recipient. `emails.email_account` records where each message + * landed, so the composer can name the right one and this can honour it. + * + * `all` is not a sender. It falls through to the default, because "send from every account" is not a thing + * anybody means — and a unified compose with no account chosen has to pick one rather than refuse. + */ async function getSmtpTransport( userId: number, + accountParam?: string, ): Promise<{ transport: ReturnType; from: string }> { const accounts = await getEmailAccounts(userId); - const acct = accounts.find((a) => a.enabled) ?? accounts[0]; + const asked = + accountParam && accountParam !== 'all' && Number.isFinite(Number(accountParam)) + ? accounts.find((a) => a.id === Number(accountParam)) + : undefined; + const acct = asked ?? accounts.find((a) => a.enabled) ?? accounts[0]; if (!acct) throw errors.BAD_REQUEST('No email account configured'); + if (accountParam && accountParam !== 'all' && !asked) { + // Naming an account that is not yours must not quietly send as you. Refusing is the only safe answer. + throw errors.BAD_REQUEST('That mail account does not exist on this login'); + } const pass = (acct.credentials as Record | null)?.password as string | undefined; if (!pass) throw errors.BAD_REQUEST('This account has no SMTP password — sending needs an app-password account'); // Gmail: smtp.gmail.com:465 (SSL). Derive from the IMAP host for other providers. @@ -60,7 +81,11 @@ emailRouter.post('/send', async (ctx) => { const html = str(form.html).trim(); const inReplyTo = str(form.inReplyTo).trim(); - const { transport, from } = await getSmtpTransport(user.id); + // The composer names the account; a reply names the one the original arrived on. + const { transport, from } = await getSmtpTransport( + user.id, + typeof form.account === 'string' ? form.account : ctx.req.query('account'), + ); await transport.sendMail({ from, to, @@ -79,7 +104,7 @@ emailRouter.post('/send', async (ctx) => { emailRouter.get('/contacts', async (ctx) => { const user = ctx.get('user'); const like = `%${(ctx.req.query('q') ?? '').trim().toLowerCase()}%`; - const db = await openUserEmailDb(user.email, user.id); + const db = await openUserEmailDb(user.email, user.id, ctx.req.query('account')); if (!db) return ctx.json([]); try { const rows = db @@ -161,13 +186,32 @@ emailRouter.get('/search', async (ctx) => { const offset = (page - 1) * limit; if (!q) return ctx.json({ messages: [], total: 0 }); - const db = await openUserEmailDb(user.email, user.id); - if (!db) return ctx.json({ messages: [], total: 0 }); + const scope = await resolveMailScope(user.id, ctx.req.query('account')); + const opened = openScope(user.email, scope); + if (!opened.length) return ctx.json({ messages: [], total: 0 }); try { - const { rows, total } = searchEmails(db, q, limit, offset); - return ctx.json({ messages: rows.map(rowToSummary), total }); + // `searchEmails` is reused verbatim, per mailbox. It has to be: `emails_fts` is a per-database FTS5 + // virtual table and `MATCH` cannot be aimed across an ATTACH boundary, so a unified search is a merge + // of local searches rather than one query. The upside is that the operator parser, the OR branching + // and every FTS detail stay in one place and are not reimplemented for the unified case. + let total = 0; + const merged = queryAcross( + opened, + (db, take) => { + const { rows, total: t } = searchEmails(db, q, take, 0); + total += t; + return rows as unknown as Array>; + }, + { sortBy: (row) => String(row.date ?? ''), limit, offset }, + ); + const messages = merged.map((row) => ({ + ...rowToSummary(row), + account: row.accountEmail, + accountId: row.accountId, + })); + return ctx.json({ messages, total }); } finally { - db.close(); + for (const { db } of opened) db.close(); } }); @@ -190,31 +234,56 @@ emailRouter.get('/messages', async (ctx) => { const { where: folderWhere, params: folderParams } = folderFilter(folder); - const db = await openUserEmailDb(user.email, user.id); - if (!db) return ctx.json({ messages: [], total: 0 }); + const scope = await resolveMailScope(user.id, ctx.req.query('account')); + const opened = openScope(user.email, scope); + if (!opened.length) return ctx.json({ messages: [], total: 0 }); try { // One row per conversation: the latest message in each thread within this folder, plus the // thread's message count and how many are unread. COALESCE guards any un-backfilled rows. - const rows = db - .query( - `SELECT * FROM ( - SELECT e.*, - COUNT(*) OVER (PARTITION BY COALESCE(thread_id, id)) AS thread_count, - SUM(CASE WHEN read = 0 THEN 1 ELSE 0 END) OVER (PARTITION BY COALESCE(thread_id, id)) AS thread_unread, - ROW_NUMBER() OVER (PARTITION BY COALESCE(thread_id, id) ORDER BY date DESC, id DESC) AS rn - FROM emails e - WHERE ${folderWhere} - ) WHERE rn = 1 - ORDER BY date DESC LIMIT ? OFFSET ?`, - ) - .all(...folderParams, limit, offset) as Record[]; - const countRow = db - .query(`SELECT COUNT(DISTINCT COALESCE(thread_id, id)) as total FROM emails WHERE ${folderWhere}`) - .get(...folderParams) as { total: number }; - const messages = rows.map(rowToSummary); - return ctx.json({ messages, total: countRow.total }); + // + // Unchanged from the single-account version, and run per mailbox rather than rewritten to span them. + // Threading is per account by definition — a conversation does not continue across two mailboxes — so + // collapsing threads locally and merging the collapsed rows is both cheaper and more correct than any + // cross-database window function would be. + const listOne = (db: (typeof opened)[number]['db'], take: number) => + db + .query( + `SELECT * FROM ( + SELECT e.*, + COUNT(*) OVER (PARTITION BY COALESCE(thread_id, id)) AS thread_count, + SUM(CASE WHEN read = 0 THEN 1 ELSE 0 END) OVER (PARTITION BY COALESCE(thread_id, id)) AS thread_unread, + ROW_NUMBER() OVER (PARTITION BY COALESCE(thread_id, id) ORDER BY date DESC, id DESC) AS rn + FROM emails e + WHERE ${folderWhere} + ) WHERE rn = 1 + ORDER BY date DESC LIMIT ?`, + ) + .all(...folderParams, take) as Record[]; + + const countOne = (db: (typeof opened)[number]['db']) => + ( + db + .query(`SELECT COUNT(DISTINCT COALESCE(thread_id, id)) as total FROM emails WHERE ${folderWhere}`) + .get(...folderParams) as { total: number } + ).total; + + const merged = queryAcross(opened, listOne, { + sortBy: (row) => String(row.date ?? ''), + limit, + offset, + }); + const total = opened.reduce((sum, entry) => sum + countOne(entry.db), 0); + + // `account` rides on every row so a unified list can label who it arrived as, and so a reply can go + // out from the right mailbox without a second lookup. + const messages = merged.map((row) => ({ + ...rowToSummary(row), + account: row.accountEmail, + accountId: row.accountId, + })); + return ctx.json({ messages, total }); } finally { - db.close(); + for (const { db } of opened) db.close(); } }); @@ -248,7 +317,10 @@ emailRouter.get('/messages/:id', async (ctx) => { const user = ctx.get('user'); const id = ctx.req.param('id'); - const db = await openUserEmailDb(user.email, user.id); + // Which mailbox holds it. Addressed by id, so the account cannot be assumed: a unified list returns ids + // from every mailbox, and opening the first would 404 on mail that is plainly on screen. + const holder = await openHolding(user.email, user.id, ctx.req.query('account'), id); + const db = holder; if (!db) return ctx.text('Not found', 404); try { const row = db.query('SELECT * FROM emails WHERE id = ? AND deleted = 0').get(id) as Record | null; @@ -264,7 +336,10 @@ emailRouter.get('/thread/:id', async (ctx) => { const user = ctx.get('user'); const id = ctx.req.param('id'); - const db = await openUserEmailDb(user.email, user.id); + // Which mailbox holds it. Addressed by id, so the account cannot be assumed: a unified list returns ids + // from every mailbox, and opening the first would 404 on mail that is plainly on screen. + const holder = await openHolding(user.email, user.id, ctx.req.query('account'), id); + const db = holder; if (!db) return ctx.text('Not found', 404); try { const head = db.query('SELECT thread_id, subject FROM emails WHERE id = ? AND deleted = 0').get(id) as { @@ -289,7 +364,10 @@ emailRouter.patch('/thread/:id/read', async (ctx) => { const user = ctx.get('user'); const id = ctx.req.param('id'); - const db = await openUserEmailDb(user.email, user.id); + // Which mailbox holds it. Addressed by id, so the account cannot be assumed: a unified list returns ids + // from every mailbox, and opening the first would 404 on mail that is plainly on screen. + const holder = await openHolding(user.email, user.id, ctx.req.query('account'), id); + const db = holder; if (!db) return ctx.text('Not found', 404); try { const head = db.query('SELECT thread_id FROM emails WHERE id = ?').get(id) as { thread_id: string | null } | null; @@ -307,7 +385,10 @@ emailRouter.post('/messages/:id/attachments/:index/extract', async (ctx) => { const id = ctx.req.param('id'); const index = Number(ctx.req.param('index')); - const db = await openUserEmailDb(user.email, user.id); + // Which mailbox holds it. Addressed by id, so the account cannot be assumed: a unified list returns ids + // from every mailbox, and opening the first would 404 on mail that is plainly on screen. + const holder = await openHolding(user.email, user.id, ctx.req.query('account'), id); + const db = holder; if (!db) return ctx.text('Attachment not found', 404); try { const row = db.query('SELECT filename, content FROM attachments WHERE email_id = ? AND idx = ?').get(id, index) as { @@ -339,7 +420,10 @@ emailRouter.patch('/messages/:id/read', async (ctx) => { const user = ctx.get('user'); const id = ctx.req.param('id'); - const db = await openUserEmailDb(user.email, user.id); + // Which mailbox holds it. Addressed by id, so the account cannot be assumed: a unified list returns ids + // from every mailbox, and opening the first would 404 on mail that is plainly on screen. + const holder = await openHolding(user.email, user.id, ctx.req.query('account'), id); + const db = holder; if (!db) return ctx.json({ ok: true }); try { db.run('UPDATE emails SET read = 1 WHERE id = ? AND read = 0', [id]); @@ -353,7 +437,10 @@ emailRouter.delete('/messages/:id', async (ctx) => { const user = ctx.get('user'); const id = ctx.req.param('id'); - const db = await openUserEmailDb(user.email, user.id); + // Which mailbox holds it. Addressed by id, so the account cannot be assumed: a unified list returns ids + // from every mailbox, and opening the first would 404 on mail that is plainly on screen. + const holder = await openHolding(user.email, user.id, ctx.req.query('account'), id); + const db = holder; if (!db) return ctx.text('Not found', 404); try { const result = db.run('UPDATE emails SET deleted = 1 WHERE id = ? AND deleted = 0', [id]); @@ -367,13 +454,18 @@ emailRouter.delete('/messages/:id', async (ctx) => { emailRouter.get('/sync-status', async (ctx) => { const user = ctx.get('user'); - const db = await openUserEmailDb(user.email, user.id); - if (!db) return ctx.json({ lastSyncAt: null }); + const scope = await resolveMailScope(user.id, ctx.req.query('account')); + const opened = openScope(user.email, scope); + if (!opened.length) return ctx.json({ lastSyncAt: null }); try { - const lastSyncAt = getSyncMeta(db, 'last_sync_at'); + // Across mailboxes the honest answer is the OLDEST, not the newest. "Synced as of" is a claim about + // everything on screen, and it stops being true at the laggard — reporting the freshest would say the + // unified inbox is current while one account has not been reached for a day. + const stamps = opened.map(({ db }) => getSyncMeta(db, 'last_sync_at')).filter((v): v is string => !!v); + const lastSyncAt = stamps.length === opened.length && stamps.length ? stamps.sort()[0] : null; return ctx.json({ lastSyncAt }); } finally { - db.close(); + for (const { db } of opened) db.close(); } }); @@ -383,7 +475,7 @@ emailRouter.get('/stats', async (ctx) => { const { where: folderWhere, params: folderParams } = folderFilter(folder); - const db = await openUserEmailDb(user.email, user.id); + const db = await openUserEmailDb(user.email, user.id, ctx.req.query('account')); if (!db) return ctx.json({ total: 0, byDomain: [], bySender: [] }); try { const total = ( @@ -409,7 +501,7 @@ emailRouter.get('/stats', async (ctx) => { emailRouter.get('/labels', async (ctx) => { const user = ctx.get('user'); - const db = await openUserEmailDb(user.email, user.id); + const db = await openUserEmailDb(user.email, user.id, ctx.req.query('account')); if (!db) return ctx.json({ labels: [] }); try { const rows = db.query('SELECT labels FROM emails WHERE deleted = 0 AND labels IS NOT NULL').all() as Array<{ diff --git a/sidecar/store.ts b/sidecar/store.ts index 817b238..ffc84b7 100644 --- a/sidecar/store.ts +++ b/sidecar/store.ts @@ -160,12 +160,31 @@ export function openEmailDb(ownerEmail: string, accountEmail: string): Database } /** - * Open the email DB for a user's configured account. Emails are stored per account - * (email_accounts//emails.db); for now we use the user's first account. Returns null if the - * user has no email account configured yet. + * Open ONE mailbox — the account asked for, or the first enabled one when nothing is asked. + * + * `accountParam` is the `?account=` query value: an account id, `all`, or absent. This used to take no + * parameter at all and always answer with the first enabled account, which is how a second mail account + * could be added, sync a full database, and never be readable — thirteen routes looked at the first one + * and nothing said so. + * + * `all` resolves to the first account HERE rather than throwing, because the routes that can genuinely + * span mailboxes do not come through this function: they use `openScope`/`queryAcross` in `mail-scope.ts`. + * A route still using this one is a route whose answer is per mailbox — a contact list, a sync status — + * and giving it the first account is the same answer it gave yesterday. */ -export async function openUserEmailDb(ownerEmail: string, userId: number): Promise { +export async function openUserEmailDb( + ownerEmail: string, + userId: number, + accountParam?: string, +): Promise { const accounts = await getEmailAccounts(userId); + if (accountParam && accountParam !== 'all') { + const id = Number(accountParam); + // Scoped to this user by `getEmailAccounts`, so an id belonging to somebody else finds nothing rather + // than opening their mail. + const asked = Number.isFinite(id) ? accounts.find((a) => a.id === id) : undefined; + return asked ? openEmailDb(ownerEmail, asked.email) : null; + } const account = accounts.find((a) => a.enabled) ?? accounts[0]; return account ? openEmailDb(ownerEmail, account.email) : null; }