diff --git a/.claude/TODO/n-plus-1-contact-log-types-lookup.md b/.claude/TODO/n-plus-1-contact-log-types-lookup.md deleted file mode 100644 index 0cc8c61..0000000 --- a/.claude/TODO/n-plus-1-contact-log-types-lookup.md +++ /dev/null @@ -1,56 +0,0 @@ -# TODO: N+1 lookup fetch in `getContactLogsByContactId` - -**Created:** 2026-08-21 -**Severity:** Low — performance only, no correctness impact. -**Status:** Open. Trivially fixable; documented rather than fixed to keep the coverage work behavior-neutral. - -## Symptom - -`src/components/contact-lookup-details/actions.ts:49-64` calls -`contactLogService.getContactLogTypes()` **inside** the `logs.map()` callback: - -```ts -const logsWithTypes = await Promise.all( - logs.map(async (log) => { - let contactLogType: string | null = null; - if (log.Contact_Log_Type_ID) { - const types = await contactLogService.getContactLogTypes(); // <-- per log - const type = types.find(t => t.Contact_Log_Type_ID === log.Contact_Log_Type_ID); - contactLogType = type?.Contact_Log_Type || null; - } - return { ...log, Contact_Log_Type: contactLogType } as ContactLogDisplay; - }) -); -``` - -For a contact with 50 logs that have a type set, that is 50 identical fetches of the same small -lookup table on every page load. - -## Fix - -Hoist the call above the loop and build a `Map` once: - -```ts -const types = await contactLogService.getContactLogTypes(); -const typeById = new Map(types.map(t => [t.Contact_Log_Type_ID, t.Contact_Log_Type])); -const logsWithTypes = logs.map(log => ({ - ...log, - Contact_Log_Type: log.Contact_Log_Type_ID ? typeById.get(log.Contact_Log_Type_ID) ?? null : null, -})) as ContactLogDisplay[]; -``` - -Since the map becomes synchronous, `Promise.all` goes away too. - -Alternatively (or additionally) memoize `getContactLogTypes()` in `ContactLogService` — it is a -lookup table that changes rarely, and other callers would benefit. - -## Why the existing tests missed it - -`contact-lookup-details/actions.ts` is at 100% statements / 90.9% branches. The test mocks -`getContactLogTypes` and never asserts a call count, so the inefficiency is invisible to the suite. -When fixing, add `expect(getContactLogTypes).toHaveBeenCalledTimes(1)` with a multi-log fixture so it -cannot regress. - -## Related - -- `.claude/docs/TestCoverage.md` §7.6 diff --git a/.claude/docs/TestCoverage.md b/.claude/docs/TestCoverage.md index 686412b..53b9b26 100644 --- a/.claude/docs/TestCoverage.md +++ b/.claude/docs/TestCoverage.md @@ -29,7 +29,7 @@ Three things matter more than the headline number: |---|---| | **Measurement was inflated ~2.2×.** With no explicit `coverage.include`, every file no test imported dropped out of the denominator. | **Fixed.** `vitest.config.ts` now sets an explicit `include`, plus per-glob `thresholds` that fail the run on regression. | | **`testing.md` claimed 95.39% coverage** — not reproducible under any configuration. | **Fixed.** Rewritten against measured numbers, with the new mock patterns documented. | -| **Coverage was pointed away from the risk.** Two `'use server'` actions have no session check at all, and both sat at 100% line coverage. | **Documented, then fixed.** §5.1 (filter injection), §5.2/§5.3 (missing auth) and §5.4/§5.5 (missing authz, duplicated User_ID lookup) are closed. §5.6 and §5.7 remain open in `.claude/TODO/`. | +| **Coverage was pointed away from the risk.** Two `'use server'` actions have no session check at all, and both sat at 100% line coverage. | **Documented, then fixed.** §5.1 (filter injection), §5.2/§5.3 (missing auth) and §5.4/§5.5 (missing authz, duplicated User_ID lookup) are closed. §5.6 (N+1 lookup) is closed. §5.7 remains open in `.claude/TODO/`. | The shape of the original problem is worth restating, because the new number does not make it go away: **high coverage is not evidence of correctness.** The filter-injection path in §5.1 lived in a @@ -236,13 +236,27 @@ That column records who made the *contact*; since any role-holder may edit anyon editor rewrote the pastoral record's authorship. MP's audit trail still captures the editor via `$userId` in `ContactLogService`. -### 5.6 N+1 query in `getContactLogsByContactId` 🟡 +### 5.6 Resolved: N+1 query in `getContactLogsByContactId` ✅ -→ `.claude/TODO/n-plus-1-contact-log-types-lookup.md` +Resolved 2026-08-21. `getContactLogTypes()` was called inside `logs.map()`, so 50 logs with a type +set meant 50 identical fetches of the same lookup table. It is now fetched once, indexed into a +`Map`, and the map is synchronous, so `Promise.all` is gone. -`getContactLogTypes()` is called inside `logs.map()`. 50 logs with a type set means 50 identical -fetches of the same lookup table. The file is at 100%/100%; the test mocks the call and never asserts -a count. +The naive hoist would not have been behavior-neutral: the old code fetched the lookup table *only* +when at least one log had a type, so a contact with no logs — or only untyped ones — made no request +and could not fail on one. A `logs.some(...)` guard preserves that exactly. + +Why the old suite missed it: the file was at 100%/100% the entire time. The test mocked +`getContactLogTypes` and never asserted a call count, so the loop was invisible. The guard is now +`toHaveBeenCalledTimes(1)` against a five-log fixture, plus `not.toHaveBeenCalled()` for the +no-typed-logs and empty-logs paths. Verified by mutation: restoring the call inside the `map` fails +the count assertion and nothing else — the other four tests in that block pin behavior, not +efficiency, which is the correct split. + +Service-level memoization of `getContactLogTypes()` was considered and deliberately skipped. The +remaining callers are one per page load and one per `contact-logs.tsx` mount; caching on a +process-wide singleton would hide a newly added contact log type until restart, for a single-digit +request saving. ### 5.7 `client.ts` token lifetime ignores `expires_in` 🟡 diff --git a/src/components/contact-lookup-details/actions.test.ts b/src/components/contact-lookup-details/actions.test.ts index ca04d18..f37e10d 100644 --- a/src/components/contact-lookup-details/actions.test.ts +++ b/src/components/contact-lookup-details/actions.test.ts @@ -114,7 +114,7 @@ describe('contact-lookup-details actions', () => { { Contact_Log_Type_ID: 2, Contact_Log_Type: 'Phone' }, ]; mockGetContactLogsByContactId.mockResolvedValueOnce(mockLogs); - mockGetContactLogTypes.mockResolvedValue(mockTypes); + mockGetContactLogTypes.mockResolvedValueOnce(mockTypes); const result = await getContactLogsByContactId(42); @@ -139,6 +139,76 @@ describe('contact-lookup-details actions', () => { }); }); + // Regression guard for `.claude/TODO/n-plus-1-contact-log-types-lookup.md`. + // `getContactLogTypes()` used to be called inside the `logs.map()` callback, so + // the same small lookup table was refetched once per typed log. The call-count + // assertions below are the whole point — the pre-existing tests mocked the call + // and never counted it, which is exactly why the N+1 was invisible to the suite. + describe('contact log type lookup is fetched once', () => { + it('fetches the lookup table exactly once for many typed logs', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogsByContactId.mockResolvedValueOnce([ + { Contact_Log_ID: 1, Contact_ID: 42, Contact_Log_Type_ID: 1, Notes: 'a' }, + { Contact_Log_ID: 2, Contact_ID: 42, Contact_Log_Type_ID: 2, Notes: 'b' }, + { Contact_Log_ID: 3, Contact_ID: 42, Contact_Log_Type_ID: 1, Notes: 'c' }, + { Contact_Log_ID: 4, Contact_ID: 42, Contact_Log_Type_ID: null, Notes: 'd' }, + { Contact_Log_ID: 5, Contact_ID: 42, Contact_Log_Type_ID: 2, Notes: 'e' }, + ]); + mockGetContactLogTypes.mockResolvedValueOnce([ + { Contact_Log_Type_ID: 1, Contact_Log_Type: 'Email' }, + { Contact_Log_Type_ID: 2, Contact_Log_Type: 'Phone' }, + ]); + + const result = await getContactLogsByContactId(42); + + expect(mockGetContactLogTypes).toHaveBeenCalledTimes(1); + expect(result.map(log => log.Contact_Log_Type)).toEqual([ + 'Email', + 'Phone', + 'Email', + null, + 'Phone', + ]); + }); + + it('does not fetch the lookup table when no log has a type', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogsByContactId.mockResolvedValueOnce([ + { Contact_Log_ID: 1, Contact_ID: 42, Contact_Log_Type_ID: null, Notes: 'a' }, + { Contact_Log_ID: 2, Contact_ID: 42, Contact_Log_Type_ID: 0, Notes: 'b' }, + ]); + + const result = await getContactLogsByContactId(42); + + expect(mockGetContactLogTypes).not.toHaveBeenCalled(); + expect(result.map(log => log.Contact_Log_Type)).toEqual([null, null]); + }); + + it('does not fetch the lookup table when the contact has no logs', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogsByContactId.mockResolvedValueOnce([]); + + const result = await getContactLogsByContactId(42); + + expect(result).toEqual([]); + expect(mockGetContactLogTypes).not.toHaveBeenCalled(); + }); + + it('maps a type with an empty name to null rather than the empty string', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetContactLogsByContactId.mockResolvedValueOnce([ + { Contact_Log_ID: 1, Contact_ID: 42, Contact_Log_Type_ID: 1, Notes: 'a' }, + ]); + mockGetContactLogTypes.mockResolvedValueOnce([ + { Contact_Log_Type_ID: 1, Contact_Log_Type: '' }, + ]); + + const result = await getContactLogsByContactId(42); + + expect(result[0].Contact_Log_Type).toBeNull(); + }); + }); + describe('Non-Error rejections', () => { // Both actions end in `throw error instanceof Error ? error : new Error(...)`. // A service that rejects with a non-Error (a string from a bare `throw`, or a diff --git a/src/components/contact-lookup-details/actions.ts b/src/components/contact-lookup-details/actions.ts index f4cce2d..17f76ee 100644 --- a/src/components/contact-lookup-details/actions.ts +++ b/src/components/contact-lookup-details/actions.ts @@ -44,25 +44,26 @@ export async function getContactLogsByContactId(contactId: number): Promise { - let contactLogType: string | null = null; - - if (log.Contact_Log_Type_ID) { - const types = await contactLogService.getContactLogTypes(); - const type = types.find(t => t.Contact_Log_Type_ID === log.Contact_Log_Type_ID); - contactLogType = type?.Contact_Log_Type || null; - } - - return { - ...log, - Contact_Log_Type: contactLogType, - } as ContactLogDisplay; - }) - ); + // Transform to ContactLogDisplay with type information. + // + // The lookup table is fetched once and indexed, not once per log. The + // `some` guard keeps the previous behavior of making no request at all when + // nothing needs mapping — without it, a contact whose logs are all untyped + // would newly fail here if the lookup fetch failed. + const typeById = new Map(); + if (logs.some(log => log.Contact_Log_Type_ID)) { + const types = await contactLogService.getContactLogTypes(); + for (const type of types) { + typeById.set(type.Contact_Log_Type_ID, type.Contact_Log_Type || null); + } + } - return logsWithTypes; + return logs.map(log => ({ + ...log, + Contact_Log_Type: log.Contact_Log_Type_ID + ? typeById.get(log.Contact_Log_Type_ID) ?? null + : null, + })) as ContactLogDisplay[]; } catch (error) { console.error('Error fetching contact logs:', error); throw error instanceof Error ? error : new Error('Failed to fetch contact logs');