perf(contact-lookup-details): fetch contact log types once, not once per log

.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` 🟡
 

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

src/components/contact-lookup-details/actions.ts

@@ -44,25 +44,26 @@ export async function getContactLogsByContactId(contactId: number): Promise<Cont
     const contactLogService = await ContactLogService.getInstance();
     const logs = await contactLogService.getContactLogsByContactId(id);
 
-    // Transform to ContactLogDisplay with type information
-    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();
-          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<number, string | null>();
+    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');