Merge pull request #1307 from Open-Source-Legal/claude/resolve-issue-1295-2uMj6

Fix Rules-of-Hooks violation in UserProfileRoute

Author: JSv4

CHANGELOG.md

@@ -175,6 +175,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Rules-of-Hooks violation in `UserProfileRoute`** (Issue #1295): `frontend/src/components/routes/UserProfileRoute.tsx` called `useQuery` _after_ a conditional early return for the `!slug` redirect case, so when the same component fiber was reused across the `/profile` → `/users/:slug` redirect (e.g. both routes rendering `UserProfileRoute` in a single `Routes` tree) React threw `Rendered more hooks than during the previous render`. Production flows that unmount/remount across the redirect masked the bug, but any future refactor that kept the fiber alive would start crashing, and test suites that mounted both routes together could not exercise the full redirect → render path. Moved `useQuery` above the early-return block and pass `{ slug: slug ?? "" }` as variables while keeping the existing `skip: !slug` gate on the network call.
 - **`PdfAnnotations.undoAnnotation()` mutated the source array** (Issue #1291): `frontend/src/components/annotator/types/annotations.ts` called `this.annotations.pop()` before constructing the returned instance, silently mutating the caller's `annotations` array even though the field is declared `readonly`. Every other method on this reducer-style class returns without touching the input, so the leak was a footgun for downstream consumers of the upcoming PdfAnnotator package extraction (Issue #1283) — any reference to the pre-undo instance would observe a stale shorter array without re-rendering. Replaced `.pop()` with a non-mutating `slice(0, -1)` + index read. Updated `frontend/src/components/annotator/types/__tests__/annotations.test.ts` to remove the pinning comment and add an explicit assertion that the original instance's `annotations` array is unchanged after `undoAnnotation()`.
 - **`RelationGroup.updateForAnnotationDeletion` never pruned orphaned relations** (Issue #1292): `frontend/src/components/annotator/types/annotations.ts:49-50` computed `nowSourceEmpty` / `nowTargetEmpty` against `this.sourceIds` / `this.targetIds` (the **pre-filter** arrays) instead of the `newSourceIds` / `newTargetIds` arrays produced a few lines above. Because the "now empty" flags were identical to the "was empty" flags, all four `return undefined` branches intended to prune orphaned relations were unreachable, and the method always returned a fresh `RelationGroup` — even when `PdfAnnotations.undoAnnotation()` had just removed the relation's only source or target annotation. That left zombie relations in `PdfAnnotations.relations` with empty `sourceIds` or `targetIds` and required defensive handling downstream. Fix swaps the two comparisons to the post-filter arrays so the documented deletion conditions fire correctly. `frontend/src/components/annotator/types/__tests__/annotations.test.ts` drops the bug-pinning header note and replaces the ambiguous "prunes any relations" assertion with two explicit cases: the survivor is kept when it still has a source and a target, and a relation whose only target references the popped annotation is now removed (21/21 unit tests pass).
 - **TOCTOU race on `DocumentPath` uniqueness** (Issue #1200): `DocumentFolderService.move_document_to_folder()`, `move_documents_to_folder()`, and `delete_folder()` previously caught `IntegrityError` from the `unique_active_path_per_corpus` partial unique constraint and either bubbled it up to the caller as a "Path conflict, please retry" error (single move) or rolled back the entire batch (bulk move / folder delete). Under concurrent moves of different documents to the same target folder, two transactions could both observe a candidate path as free in `_disambiguate_path()` and race to insert it; the loser hit the partial unique index and the operation failed. New helper `DocumentFolderService._create_successor_path_with_retry()` (`opencontractserver/corpuses/folder_service.py`) wraps the deactivate-then-create pair in a savepoint and retries with a freshly disambiguated path on `IntegrityError`, treating each losing path as occupied. Up to `MAX_PATH_CREATE_RETRIES + 1` attempts (`opencontractserver/constants/document_processing.py`) run before propagating the conflict. The partial unique index added in migration `0023_documentpath_documentpathgroupobjectpermission_and_more` remains the authoritative correctness guarantee. New test classes `TestMoveDocumentIntegrityRecovery`, `TestBulkMoveIntegrityRecovery`, and `TestDeleteFolderIntegrityRecovery` cover transient-failure recovery, disambiguated retry path selection, and exhausted-retry rollback (`opencontractserver/tests/test_document_folder_service.py`).

docs/assets/images/screenshots/auto/users--profile--public.png

@@ -21,6 +21,15 @@ export const UserProfileRoute: React.FC = () => {
   const { slug } = useParams<{ slug: string }>();
   const currentUser = useReactiveVar(backendUserObj);
 
+  // Must be unconditional — skip: !slug gates the network call; see #1295.
+  const { data, loading, error } = useQuery<GetUserOutput, GetUserInput>(
+    GET_USER,
+    {
+      variables: { slug: slug ?? "" },
+      skip: !slug,
+    }
+  );
+
   // If no slug provided, redirect to current user's profile
   if (!slug) {
     if (!currentUser?.slug) {
@@ -29,14 +38,6 @@ export const UserProfileRoute: React.FC = () => {
     return <Navigate to={`/users/${currentUser.slug}`} replace />;
   }
 
-  const { data, loading, error } = useQuery<GetUserOutput, GetUserInput>(
-    GET_USER,
-    {
-      variables: { slug },
-      skip: !slug,
-    }
-  );
-
   if (loading) {
     return <ModernLoadingDisplay type="default" message="Loading profile..." />;
   }

frontend/src/components/routes/UserProfileRoute.tsx

@@ -88,11 +88,6 @@ describe("UserProfileRoute", () => {
   });
 
   it("redirects /profile to /users/<slug> when a user is logged in", async () => {
-    // NOTE: we don't mount the /users/:slug route here because
-    // UserProfileRoute has a latent Rules-of-Hooks issue when it's reused
-    // across a <Navigate> — the early-return path skips useQuery(), then
-    // once the URL changes React tries to add the hook to the same fiber.
-    // Tracked separately; the redirect itself still works in practice.
     backendUserObj({ id: "u-1", slug: "alice" } as any);
 
     render(
@@ -110,6 +105,38 @@ describe("UserProfileRoute", () => {
     expect(loc.textContent).toBe("/users/alice");
   });
 
+  it("redirects /profile and then renders /users/:slug without a Rules-of-Hooks crash (issue #1295)", async () => {
+    // Regression guard: the pre-fix code called useQuery *after* the
+    // early-return for the no-slug case, so when /profile redirected to
+    // /users/:slug the same UserProfileRoute fiber transitioned from 0 to 1
+    // hook and React threw. With useQuery hoisted unconditionally, the two
+    // renders share the same hook ordering and the redirect survives.
+    backendUserObj({ id: "u-1", slug: "alice" } as any);
+
+    render(
+      <MockedProvider
+        mocks={[
+          {
+            request: { query: GET_USER, variables: { slug: "alice" } },
+            result: { data: { userBySlug: user } },
+          },
+        ]}
+        addTypename={false}
+      >
+        <MemoryRouter initialEntries={["/profile"]}>
+          <Routes>
+            <Route path="/profile" element={<UserProfileRoute />} />
+            <Route path="/users/:slug" element={<UserProfileRoute />} />
+          </Routes>
+        </MemoryRouter>
+      </MockedProvider>
+    );
+
+    await waitFor(() => {
+      expect(screen.getByText("UserProfile:alice")).toBeInTheDocument();
+    });
+  });
+
   it("renders UserProfile with isOwnProfile=true when viewer is the profile owner", async () => {
     backendUserObj({ id: "u-1", slug: "alice" } as any);
 

frontend/src/components/routes/__tests__/UserProfileRoute.test.tsx

@@ -25,6 +25,76 @@ const mockPublicUser = {
   totalDocumentsUploaded: 10,
 };
 
+test.describe("UserProfileRoute - Hook Ordering Regression (Issue #1295)", () => {
+  test("mounts with slug under a Routes tree that also defines /profile (no 'Rendered more hooks' crash)", async ({
+    mount,
+    page,
+  }) => {
+    // This test exercises the scenario that surfaced the original bug: both
+    // the redirect route (/profile, no slug) and the render route
+    // (/users/:slug) live in the same <Routes> tree. When useQuery was below
+    // the `!slug` early return, React could (depending on fiber reuse)
+    // compare hook call counts across the two renders and throw
+    // "Rendered more hooks than during the previous render". With the fix,
+    // useQuery is called unconditionally and skip: !slug gates the network
+    // request, so the component renders without crashing on either path.
+    //
+    // NOTE: This is a narrow regression guard rather than a full reproduction
+    // of the original crash. Reproducing the exact crash in-process requires
+    // keeping the same fiber across /profile -> /users/:slug, which React
+    // Router <Routes> doesn't do (it unmounts the old element when the
+    // matched path changes). The positive assertions below (loading display
+    // for the slug path, absence of hook-count errors) prove the hook
+    // ordering is stable — exactly what the fix guarantees statically.
+    const consoleErrors: string[] = [];
+    page.on("console", (msg) => {
+      if (msg.type() === "error") consoleErrors.push(msg.text());
+    });
+
+    // Delay the mock so the render parks on the loading state, giving us
+    // positive visible evidence (not just the absence of a crash) that
+    // useQuery ran under skip:false on the slug path.
+    const mocks = [
+      {
+        request: {
+          query: GET_USER,
+          variables: { slug: "publicuser-123" },
+        },
+        delay: 5000,
+        result: {
+          data: {
+            userBySlug: mockPublicUser,
+          },
+        },
+      },
+    ];
+
+    const component = await mount(
+      <MockedProvider mocks={mocks} addTypename={false}>
+        <MemoryRouter initialEntries={["/users/publicuser-123"]}>
+          <Routes>
+            <Route path="/profile" element={<UserProfileRoute />} />
+            <Route path="/users/:slug" element={<UserProfileRoute />} />
+          </Routes>
+        </MemoryRouter>
+      </MockedProvider>
+    );
+
+    // Wait for positive DOM evidence that the slug render reached the
+    // useQuery call (skip:false branch) instead of a silent bailout.
+    await expect(page.locator("text=Loading profile...")).toBeVisible({
+      timeout: 10000,
+    });
+
+    const hookErrors = consoleErrors.filter((e) =>
+      e.includes("Rendered more hooks than during the previous render")
+    );
+    expect(hookErrors).toEqual([]);
+
+    await component.unmount();
+  });
+});
+
 test.describe("UserProfile View - Loading and Error States", () => {
   test("should show loading state while fetching user data", async ({
     mount,