Address inline review comments from PR #11

chapter-detail.tsx: wrap allChapterNames in useMemo so memberConstraints,
eventConstraints, and fundraisingConstraints only recompute when chapter
or aliases actually change, preventing unnecessary useCollection re-subscriptions.

use-firestore.ts: reset data/error/loading at the start of useDocument's
effect so stale state is cleared immediately on every docId transition
instead of persisting until the new snapshot arrives.

data-table.spec.ts:
- "third click removes sort": capture row state before first click and
  assert it matches after third click to verify the tri-state cycle
- "can change page": coerce initialText to string (same null-safety fix
  as newText), and drop the weak || newText.length > 0 fallback so the
  assertion fails when pagination genuinely does not work
- "columns can be resized": call test.fixme() when resize handle is
  absent rather than silently passing; assert widthChanged directly
  without the vacuous || newBox !== null fallback
- Remove unused initialCount variable in "clearing search shows all results"

fundraising.spec.ts: skip the archive test when search input is missing
(to avoid acting on real campaigns) and scope the archiveButton locator
to rows containing TEST_DATA_PREFIX instead of page-global .first()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: 2 people

pnaa/components/chapters/chapter-detail.tsx

@@ -122,9 +122,16 @@ export function ChapterDetail({ chapterId }: { chapterId: string }) {
   );
 
   // All chapter names to query (main + aliases)
-  const allChapterNames = chapter?.name
-    ? [chapter.name, ...(aliases as AliasRow[]).map((a) => a.aliasName)]
-    : [];
+  // Wrapped in useMemo so the array reference is stable across renders,
+  // preventing memberConstraints / eventConstraints / fundraisingConstraints
+  // from recomputing (and re-subscribing) unless chapter or aliases actually change.
+  const allChapterNames = useMemo(
+    () =>
+      chapter?.name
+        ? [chapter.name, ...(aliases as AliasRow[]).map((a) => a.aliasName)]
+        : [],
+    [chapter, aliases]
+  );
 
   const hasAliases = (aliases as AliasRow[]).length > 0;
 

pnaa/e2e/local/data-table.spec.ts

@@ -75,17 +75,20 @@ test.describe("Data Table - Column Sorting", () => {
     const header = page.locator("th").filter({ hasText: /name/i }).first();
 
     if (await header.isVisible()) {
-      // Click three times to cycle through asc -> desc -> none
+      // Capture unsorted state before any clicks
+      const initialRow = await page.locator("table tbody tr").first().textContent().catch(() => "");
+
+      // Cycle: first click (ascending), second click (descending), third click (removes sort)
       await header.click();
       await page.waitForTimeout(200);
       await header.click();
       await page.waitForTimeout(200);
       await header.click();
       await page.waitForTimeout(200);
 
-      // After third click, sort should be removed or cycled
-      const hasSortIcon = await header.locator('button svg').count() > 0;
-      expect(hasSortIcon).toBeTruthy();
+      // After third click the sort is removed — row order must be back to initial
+      const finalRow = await page.locator("table tbody tr").first().textContent().catch(() => "");
+      expect(finalRow).toBe(initialRow);
     }
   });
 });
@@ -179,7 +182,7 @@ test.describe("Data Table - Pagination", () => {
     if (await nextButton.isVisible() && await nextButton.isEnabled()) {
       // Store first row content
       const firstRow = page.locator("table tbody tr").first();
-      const initialText = await firstRow.textContent().catch(() => "");
+      const initialText = (await firstRow.textContent().catch(() => "")) ?? "";
 
       // Click next
       await nextButton.click();
@@ -190,7 +193,7 @@ test.describe("Data Table - Pagination", () => {
       const buttonDisabled = !(await nextButton.isEnabled());
       const contentChanged = newText !== initialText;
 
-      expect(contentChanged || buttonDisabled || newText.length > 0).toBeTruthy();
+      expect(contentChanged || buttonDisabled).toBeTruthy();
     }
   });
 
@@ -320,9 +323,6 @@ test.describe("Data Table - Filtering", () => {
     const searchInput = page.locator('input[placeholder*="search" i]').first();
 
     if (await searchInput.isVisible()) {
-      // Get initial count before any filtering
-      const initialCount = await page.locator("table tbody tr").count();
-
       // First filter with something unlikely to match
       await searchInput.fill("xyznonexistent999");
       await page.waitForTimeout(300);
@@ -358,25 +358,27 @@ test.describe("Data Table - Column Resizing", () => {
       '[data-testid="column-resize-handle"], .resize-handle, th [class*="resize"]'
     ).first();
 
-    if (await resizeHandle.isVisible()) {
-      // Get initial column width
-      const header = page.locator("th").first();
-      const initialBox = await header.boundingBox();
-
-      if (initialBox) {
-        // Drag resize handle
-        await resizeHandle.hover();
-        await page.mouse.down();
-        await page.mouse.move(initialBox.x + initialBox.width + 50, initialBox.y);
-        await page.mouse.up();
-
-        // Width should have changed
-        const newBox = await header.boundingBox();
+    if (!(await resizeHandle.isVisible())) {
+      // Resize handle not found — feature not yet implemented
+      test.fixme();
+      return;
+    }
 
-        // Verify resize occurred (width changed) or resize feature works
-        const widthChanged = newBox && newBox.width !== initialBox.width;
-        expect(widthChanged || newBox !== null).toBeTruthy();
-      }
+    // Get initial column width
+    const header = page.locator("th").first();
+    const initialBox = await header.boundingBox();
+
+    if (initialBox) {
+      // Drag resize handle
+      await resizeHandle.hover();
+      await page.mouse.down();
+      await page.mouse.move(initialBox.x + initialBox.width + 50, initialBox.y);
+      await page.mouse.up();
+
+      // Width must have changed for resize to be working
+      const newBox = await header.boundingBox();
+      const widthChanged = newBox !== null && newBox.width !== initialBox.width;
+      expect(widthChanged).toBeTruthy();
     }
   });
 });

pnaa/e2e/local/fundraising.spec.ts

@@ -307,19 +307,23 @@ test.describe("Fundraising - CRUD Operations", () => {
     await page.goto("/fundraising");
     await page.waitForLoadState("networkidle");
 
-    // Filter to test data to avoid archiving real campaigns
+    // Filter to test data rows — hard-skip if search is unavailable to avoid
+    // accidentally targeting real campaigns.
     const searchInput = page.locator('input[placeholder*="search" i]').first();
-    if (await searchInput.isVisible()) {
-      await searchInput.fill(TEST_DATA_PREFIX);
-      await page.waitForTimeout(500);
+    if (!(await searchInput.isVisible())) {
+      test.skip();
+      return;
     }
+    await searchInput.fill(TEST_DATA_PREFIX);
+    await page.waitForTimeout(500);
 
     // Get row count after filtering to test data
     const tableRows = page.locator("table tbody tr");
     const initialCount = await tableRows.count();
 
-    // Find an archive/delete button
-    const archiveButton = page
+    // Scope the archive button to rows that contain TEST_DATA_PREFIX only
+    const testDataRow = page.locator("table tbody tr").filter({ hasText: TEST_DATA_PREFIX }).first();
+    const archiveButton = testDataRow
       .locator('button:has-text("Archive"), button:has-text("Delete"), button[aria-label*="archive" i]')
       .first();
 

pnaa/hooks/use-firestore.ts

@@ -19,6 +19,14 @@ export function useDocument<T>(
   const [error, setError] = useState<Error | null>(null);
 
   useEffect(() => {
+    // Reset stale state immediately on every docId transition (including → undefined)
+    // so consumers never see data/error from a previous document.
+    // All three calls are batched by React 18 and are intentional here.
+    // eslint-disable-next-line react-hooks/set-state-in-effect
+    setData(null);
+    setError(null);
+    setLoading(Boolean(docId));
+
     if (!docId) return;
 
     const docRef = doc(db, collectionName, docId);
@@ -42,7 +50,7 @@ export function useDocument<T>(
     return () => unsubscribe();
   }, [collectionName, docId]);
 
-  return { data, loading: docId ? loading : false, error };
+  return { data, loading, error };
 }
 
 export function useCollection<T>(