Merge pull request #1325 from Open-Source-Legal/claude/resolve-issue-1321-zE3ic

Tighten tests per PR #1297 follow-up (issue #1321)

Author: JSv4

CHANGELOG.md

@@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **PR #1297 follow-up — tighten component tests and unit-test coverage** (Issue #1321):
+  - `frontend/src/components/documents/__tests__/DocumentRelationshipModal.test.ts`: added a test that pins the `labelType: undefined` edge case for `filterRelationshipLabels` (previously only `null` non-relationship labels were covered), so the strict-equality guard against non-`RelationshipLabel` types is explicit.
+  - `frontend/tests/ModernDocumentItem.ct.tsx`:
+    - Extended the `__reactProps$` upgrade-risk comments on `clickViaReact` / `openContextMenu` to call out the build-hash suffix explicitly — the suffix rotates on every React build, so `Object.keys(el).find(k => k.startsWith("__reactProps$"))` is the only correct lookup pattern.
+    - `openContextMenu` now falls back to a full-document scan for an element with a React `onContextMenu` prop when the `.checkbox` anchor is absent (e.g. future conditional rendering of read-only items). The primary checkbox path is unchanged.
+    - Added `NOTE` comments on the relationship-popup `toBeAttached()` assertions explaining that they only verify DOM presence (the popup uses `visibility: hidden` + hover reveal) and when to switch to `toBeVisible()` instead.
+  - `frontend/tests/DocumentRelationshipModal.ct.tsx`:
+    - Replaced the DOM-order `removeButtons.nth(1)` assertion in the "removes document from target column" test with an XPath-based walk from the pill's visible title, so re-ordering the source/target layout can no longer silently test the wrong button.
+    - Strengthened the mutation-failure negative assertion: after submitting a failing mutation, wait for the submit button to re-enable (a positive signal that the `isSubmitting` finally-block has executed) before polling `onSuccessCalled === false`. Prevents the poll from passing immediately on the initial `false` value.
+  - `frontend/tests/utils/ReactiveVarObserver.tsx`: expanded the doc comment with a three-step recipe for extending the observer to additional reactive vars, matching the existing `viewingDocument` / `editingDocument` convention.
+
 ### Fixed
 
 - **`fullDatacellList` no-args path was unbounded** (Issue #1256, follow-up to PR #1235, `config/graphql/extract_types.py:131-148`): `ExtractType.resolve_full_datacell_list` capped the `limit` and `offset-only` branches at `MAX_FULL_DATACELL_LIST_LIMIT` but returned the entire queryset when called with no arguments. Authenticated callers hitting `fullDatacellList` directly (no `limit`, no `offset`) could bypass the payload bound. Collapsed all three code paths so every call — no-args, offset-only, or `limit`+`offset` — returns `qs[start : start + min(limit_or_max, MAX_FULL_DATACELL_LIST_LIMIT)]`. The embed UI is unaffected (it already passes `limit: 500`); direct API callers now receive at most 500 cells and must paginate via `offset` to walk the rest. Added regression test `test_full_datacell_list_no_args_capped_at_server_max` in `opencontractserver/tests/test_extract_queries.py` which creates 501 cells and asserts the no-args response returns exactly `MAX_FULL_DATACELL_LIST_LIMIT` while `datacellCount` reports the true total.

frontend/src/components/documents/__tests__/DocumentRelationshipModal.test.ts

@@ -6,7 +6,7 @@ const makeLabel = (
   overrides: Partial<{
     id: string;
     text: string | null;
-    labelType: string;
+    labelType: string | undefined;
   }> = {}
 ) => ({
   id: "l-1",
@@ -36,6 +36,18 @@ describe("filterRelationshipLabels", () => {
     expect(out.map((l) => l.id)).toEqual(["l-1", "l-3"]);
   });
 
+  it("excludes labels whose labelType is undefined (not just null)", () => {
+    // Strict equality `=== LabelType.RelationshipLabel` excludes both null
+    // and undefined; this exercises the undefined edge-case explicitly so a
+    // future refactor doesn't accidentally start accepting unset labelType.
+    const labels = [
+      makeLabel({ id: "l-1", text: "references" }),
+      makeLabel({ id: "l-2", text: "header", labelType: undefined }),
+    ];
+    const out = filterRelationshipLabels(labels, "", true);
+    expect(out.map((l) => l.id)).toEqual(["l-1"]);
+  });
+
   it("returns all relationship labels unfiltered when searchTerm is empty", () => {
     const labels = [
       makeLabel({ id: "l-1", text: "references" }),

frontend/tests/DocumentRelationshipModal.ct.tsx

@@ -288,10 +288,15 @@ test.describe("DocumentRelationshipModal — state transitions", () => {
 
     await page.waitForSelector('text="Target Document 1"', { timeout: 10000 });
 
-    // The target pill's Remove button is the second one on the page
-    // (source has one Remove, target has one Remove)
-    const removeButtons = page.locator('button[title="Remove"]');
-    await removeButtons.nth(1).click();
+    // Scope the Remove click to the target pill by walking up from the
+    // pill's visible title to the first ancestor that contains a Remove
+    // button. This avoids the earlier DOM-order `.nth(1)` approach which
+    // would silently click the wrong button if the source/target layout
+    // ever changes.
+    const targetPill = page
+      .getByText("Target Document 1")
+      .locator('xpath=ancestor::*[.//button[@title="Remove"]][1]');
+    await targetPill.locator('button[title="Remove"]').click();
 
     await expect(page.getByText("No target documents")).toBeVisible();
   });
@@ -718,15 +723,20 @@ test.describe("DocumentRelationshipModal — submit", () => {
     const submitBtn = page.getByRole("button", { name: /Create Relationship/ });
     await submitBtn.click();
 
-    // Modal should still be open after the failed mutation (the failure path
-    // does not close the modal). Use a visibility assertion instead of a fixed
-    // sleep so we only wait as long as needed for the mutation to settle.
+    // Wait for the mutation to settle. When the mutation fails the submit
+    // button re-enables (the component flips `isSubmitting` back off) and
+    // the modal stays open — both are positive signals the failure path
+    // has fully executed. Waiting for them before the negative assertion
+    // gives `onSuccessCalled` a real window in which it *could* have been
+    // invoked; without this stabilization the poll passes immediately on
+    // the initial `false` value and provides no timing protection.
     await expect(page.getByText("Link Documents")).toBeVisible({
       timeout: 5000,
     });
+    await expect(submitBtn).toBeEnabled({ timeout: 5000 });
 
     // onSuccess should NOT be invoked when all mutations fail.
-    await expect.poll(() => onSuccessCalled, { timeout: 3000 }).toBe(false);
+    await expect.poll(() => onSuccessCalled, { timeout: 2000 }).toBe(false);
   });
 });
 

frontend/tests/ModernDocumentItem.ct.tsx

@@ -55,8 +55,13 @@ function renderWithProviders(
  * Invoke the React onClick handler directly via the __reactProps$ fiber.
  *
  * Tested against React 18.x. The `__reactProps$` key is a React internal with
- * no semver guarantee — if this breaks after a React upgrade, check whether
- * DndContext now forwards pointer events to children and remove this shim.
+ * no semver guarantee. Two things can silently break this shim on upgrade:
+ *   1. The property name stem (`__reactProps$`) could be renamed/removed.
+ *   2. The suffix after `$` is a build-specific random ID that changes on
+ *      every React build (including minor/patch versions), so we must never
+ *      hard-code the full key — always find it via `Object.keys(el).find(...)`.
+ * If this breaks after a React upgrade, check whether DndContext now forwards
+ * pointer events to children and remove this shim.
  */
 async function clickViaReact(
   page: import("@playwright/test").Page,
@@ -218,7 +223,12 @@ test.describe("ModernDocumentItem — card view rendering", () => {
 
     // Number is rendered in the badge
     await expect(page.getByText("2").first()).toBeVisible();
-    // Popup text is present in the DOM even before hover (visibility:hidden)
+    // NOTE: toBeAttached() verifies DOM presence only — the relationship
+    // popup uses `visibility: hidden` by default and reveals on hover, so
+    // the nodes are always in the tree. If the popup ever switches to
+    // conditional rendering (mount-on-hover), these assertions would still
+    // pass while the feature breaks; revisit and use toBeVisible() after a
+    // hover step in that case.
     await expect(
       page.locator('text="2 Linked Documents"').first()
     ).toBeAttached();
@@ -326,7 +336,10 @@ test.describe("ModernDocumentItem — list view rendering", () => {
       mount
     );
 
-    // "1 Linked Document" (singular) should be in the popup DOM
+    // "1 Linked Document" (singular) should be in the popup DOM.
+    // NOTE: toBeAttached() verifies DOM presence only — see the card-view
+    // relationship popup test above for the rationale behind not using
+    // toBeVisible() here.
     await expect(
       page.locator('text="1 Linked Document"').first()
     ).toBeAttached();
@@ -664,36 +677,53 @@ test.describe("ModernDocumentItem — processing state", () => {
 // DndContext pointer listeners don't intercept contextmenu, but to avoid
 // relying on coordinate-based click targeting we walk up the DOM from a known
 // element and invoke the React onContextMenu prop directly. Same React 18.x
-// `__reactProps$` caveat as clickViaReact applies.
+// `__reactProps$` caveat as clickViaReact applies (including the build-hash
+// suffix: never hard-code the full key, always discover it via Object.keys).
 // ---------------------------------------------------------------------------
 
 async function openContextMenu(page: import("@playwright/test").Page) {
   await page.evaluate(() => {
-    // Walk up from the title element to find an ancestor with onContextMenu.
+    const dispatch = (el: HTMLElement): boolean => {
+      const propsKey = Object.keys(el).find((k) =>
+        k.startsWith("__reactProps$")
+      );
+      if (!propsKey) return false;
+      const props = (el as any)[propsKey];
+      if (typeof props?.onContextMenu !== "function") return false;
+      props.onContextMenu({
+        preventDefault: () => {},
+        stopPropagation: () => {},
+        clientX: 100,
+        clientY: 100,
+      });
+      return true;
+    };
+
+    // Prefer walking up from the checkbox — it's nested inside the item root
+    // and guarantees we target the correct card/list ancestor. If the
+    // checkbox is ever conditionally hidden (e.g. future read-only view),
+    // fall back to scanning the entire document for the first element with
+    // an onContextMenu React prop, which will be the item root because
+    // nothing else in the tree attaches one.
     let el: HTMLElement | null = document.querySelector(
       ".checkbox"
     ) as HTMLElement | null;
     while (el) {
-      const propsKey = Object.keys(el).find((k) =>
-        k.startsWith("__reactProps$")
-      );
-      if (propsKey) {
-        const props = (el as any)[propsKey];
-        if (typeof props?.onContextMenu === "function") {
-          props.onContextMenu({
-            preventDefault: () => {},
-            stopPropagation: () => {},
-            clientX: 100,
-            clientY: 100,
-          });
-          return;
-        }
-      }
+      if (dispatch(el)) return;
       el = el.parentElement;
     }
+
+    // Fallback: scan every element in the document for a React onContextMenu
+    // handler. Order is document order, so the outermost handler wins — which
+    // is fine because only the item root attaches one.
+    const all = document.querySelectorAll<HTMLElement>("*");
+    for (const candidate of Array.from(all)) {
+      if (dispatch(candidate)) return;
+    }
+
     throw new Error(
-      "openContextMenu: no onContextMenu handler found walking up from .checkbox — " +
-        "is the component rendered with the checkbox present?"
+      "openContextMenu: no element in the document carries a React " +
+        "onContextMenu prop — is the ModernDocumentItem rendered?"
     );
   });
 }

frontend/tests/utils/ReactiveVarObserver.tsx

@@ -7,6 +7,13 @@ import { viewingDocument, editingDocument } from "../../src/graphql/cache";
  * and the component under test (browser) see separate instances. Mounting
  * this observer inside the tree exposes the browser-side values via data
  * attributes, which the test can query without crossing the process boundary.
+ *
+ * Currently observes `viewingDocument` and `editingDocument`. To expose an
+ * additional reactive var, follow the same pattern:
+ *   1. Import the var from `../../src/graphql/cache`.
+ *   2. Subscribe with `useReactiveVar(...)` inside the component.
+ *   3. Render its value as a `data-<name>-id` attribute on the same element.
+ * Tests then read it with `page.getByTestId("rv-observer").getAttribute(...)`.
  */
 export const ReactiveVarObserver: React.FC = () => {
   const viewing = useReactiveVar(viewingDocument);