Merge pull request #1383 from Open-Source-Legal/feature/centralize-extract-and-user-route-resolution

Centralize extract and user-profile route resolution

Author: JSv4

CHANGELOG.md

@@ -28,6 +28,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **Centralized extract and user-profile route resolution in `CentralRouteManager`** (`frontend/src/routing/CentralRouteManager.tsx`, `frontend/src/components/routes/ExtractDetailRoute.tsx`, `frontend/src/components/routes/UserProfileRoute.tsx`, new `frontend/src/components/routes/ProfileRedirect.tsx`, `frontend/src/utils/navigationUtils.ts`, `frontend/src/graphql/cache.ts`): `ExtractDetailRoute` and `UserProfileRoute` were each calling `useParams()`, running their own GraphQL resolution queries, and writing entity reactive vars — duplicating the four-phase flow documented in `docs/frontend/routing_system.md` and racing the manager during back-navigation. Both are now thin consumers that read `openedExtract` / `openedUser` / `routeLoading` / `routeError`. `parseRoute` learned `/users/:slug` and `/extracts/:extractId`; the manager added a `GET_USER` lazy query and a Phase 1 user branch alongside the existing extract handling. New `openedUser` reactive var (typed `OpenedUserProfile`) joins the routing-owned set. `/profile` is now served by a small `ProfileRedirect` that uses `backendUserObj` to redirect to `/users/<slug>` — auth-driven, not URL-driven, so legitimately outside the manager. Smaller fixes that ride along: `views/Corpuses.tsx:1807` swaps `window.history.replaceState` for `navigate({ pathname, search }, { replace: true })` so query-param mutations stay inside React Router; redundant `openedLabelset(null)` calls in `LabelSetDetailPage` and `LabelSetLandingRoute` handlers were removed (Phase 1 already clears the var on browse navigation).
+  - Test coverage: 8 new `parseRoute` tests, 3 new manager tests (user resolve, user not-found, extract by id), updated `beforeEach` to reset `openedExtract` / `openedUser`. New `frontend/src/routing/__tests__/centralRouteDiscipline.test.ts` is a static regression test that grep-walks `frontend/src/` and fails if any production file outside the manager and `cache.ts` SETs one of the 15 routing-owned reactive vars — caught three pre-existing `openedLabelset` writes during development that were also fixed in this PR. New `frontend/tests/e2e/user-and-extract-routes.spec.ts` deep-links `/users/<slug>`, verifies the `/profile` redirect, and exercises the dumb-consumer error path on `/extracts/<unknown-id>`. `tests/e2e/helpers.ts` `VIEWS` catalog now includes `/users/admin` so the existing login-and-navigation walk also covers the user route.
+  - Doc updates: `docs/frontend/routing_system.md` route-pattern table now lists `/users/:slug`, `/extracts/:extractId`, and `/label_sets/:labelsetId`; the "ONLY CentralRouteManager may SET" list and the four critical RULE blocks were extended to include `openedThread`, `openedLabelset`, and `openedUser`; the new discipline test is referenced as the CI enforcement mechanism.
+- **Pre-existing `tsc` failure on `frontend/src/components/corpuses/caml/CamlDirectiveRenderer.tsx`** resolved by refreshing `@os-legal/caml-react` to `0.1.0` (latest on the registry). The lockfile had been pinned to `0.0.1`, whose published `dist/index.d.ts` lacks the `resolveImageSrc` prop that the consumer passes through; `0.1.0` ships the prop on `CamlArticleProps`, `CamlChapterRendererProps`, and the block-renderer prop interfaces. `yarn upgrade @os-legal/caml-react@^0.1.0 @os-legal/caml@^0.1.0` updates the lockfile; no code changes needed.
 - **Simplified `RelationGroup.updateForAnnotationDeletion` pruning logic** (Issue #1317, follow-up to #1314, `frontend/src/components/annotator/types/annotations.ts:40-60`): The method previously branched on four near-duplicate conditions (`sourceEmpty && nowTargetEmpty`, `targetEmpty && nowSourceEmpty`, `!sourceEmpty && nowSourceEmpty`, `!targetEmpty && nowTargetEmpty`) each returning `undefined`. All four are equivalent to a single `nowSourceEmpty || nowTargetEmpty` check (`filter` is monotonic, so an originally-empty side stays empty after filtering). Collapsed the branches and removed the now-unused `sourceEmpty` / `targetEmpty` locals per the project's DRY guideline. Behavior is unchanged; existing regression tests in `frontend/src/components/annotator/types/__tests__/annotations.test.ts` still pass unmodified, confirming the simplification is semantics-preserving.
 - **Frontend Vitest unit coverage provider switched from V8 to Istanbul** (`frontend/vite.config.ts:210-230`, `frontend/package.json:154`, `frontend/yarn.lock`): The merged `frontend` Codecov flag was landing at ~44% even though `frontend-component` alone was at ~61% on the same code — impossible for a union-aggregated metric unless the three per-suite uploads were measuring on different yardsticks. Root cause: Vitest's `@vitest/coverage-v8` provider emits ~183 `DA:` records per source file (~86k across ~480 files) because V8's native coverage API reports hits for nearly every executable line — imports, declarations, block-closing `}`, etc. — as needed for engine profiling and DevTools. `vite-plugin-istanbul` (used by Playwright CT and E2E suites) emits ~61 `DA:` per file (~27k total) because it only instruments statements. Same code, ~3× denominator mismatch. When Codecov unions multiple uploads under one flag it keeps V8's larger line-number set; Istanbul hits from CT/E2E land on a subset of those line numbers and can never close the gap. Swapped the unit-coverage provider to Istanbul so all three suites report on the same universe. Swapped the `@vitest/coverage-v8` devDep for `@vitest/coverage-istanbul` at the same `^3.1.2` version to match the `vitest` major.minor, changed `coverage.provider` to `"istanbul"` in the Vitest config, regenerated `yarn.lock`, and updated the `all: true` rationale comment to stop singling out V8. The per-suite `flags:` tagging in `frontend.yml` / `frontend-e2e.yml` is unchanged — server-side aggregation still handles the merge. Verified locally: the new unit lcov reports ~451 `SF:` and ~24k `DA:` (previously ~86k), matching the CT/E2E scale. Trade-off: unit-test runtime under coverage grows somewhat (Istanbul transforms source pre-execution), likely 10–30s on this suite. `tsc --noEmit` clean.
 

docs/frontend/routing_system.md

@@ -36,6 +36,9 @@ The OpenContracts routing system follows a **centralized architecture** where **
 - `openedCorpus()`
 - `openedDocument()`
 - `openedExtract()`
+- `openedThread()`
+- `openedLabelset()`
+- `openedUser()`
 
 **URL-Driven State (set by Phase 2, watched by Phase 4):**
 - `selectedAnnotationIds()`
@@ -54,15 +57,11 @@ The OpenContracts routing system follows a **centralized architecture** where **
 **The Correct Pattern:**
 Components wanting to change URL-driven state must use utility functions that update the URL. CentralRouteManager Phase 2 will detect the URL change and set the reactive var. This maintains unidirectional data flow: Component → URL → CentralRouteManager → Reactive Var → Component.
 
-This is non-negotiable. Violations cause infinite loops, route jittering, competing state updates, and unpredictable behavior. During development, we systematically removed all violations from:
-- `CorpusDocumentCards.tsx` (caused infinite loop bug)
-- `DocumentKnowledgeBase.tsx` (4 violations)
-- `FloatingDocumentControls.tsx` (1 violation)
-- `NavMenu.tsx` / `MobileNavMenu.tsx` (clearing on menu clicks)
-- `CorpusBreadcrumbs.tsx` (manual clearing)
-- `Corpuses.tsx` (3 violations)
+This is non-negotiable. Violations cause infinite loops, route jittering, competing state updates, and unpredictable behavior. The static `centralRouteDiscipline.test.ts` regression guard enforces this rule today, and the historical violations it was built to prevent (URL bypasses in `CorpusDocumentCards.tsx`, `DocumentKnowledgeBase.tsx`, `FloatingDocumentControls.tsx`, `NavMenu.tsx`/`MobileNavMenu.tsx`, `CorpusBreadcrumbs.tsx`, and `Corpuses.tsx`) have all been removed.
 
-**If you find yourself writing `openedCorpus(someValue)`, `openedDocument(someValue)`, or `openedExtract(someValue)` anywhere except `CentralRouteManager.tsx`, STOP. You are introducing a bug.**
+**If you find yourself writing `openedCorpus(someValue)`, `openedDocument(someValue)`, `openedExtract(someValue)`, `openedThread(someValue)`, `openedLabelset(someValue)`, or `openedUser(someValue)` anywhere except `CentralRouteManager.tsx`, STOP. You are introducing a bug.**
+
+> A static regression test (`frontend/src/routing/__tests__/centralRouteDiscipline.test.ts`) grep-walks `frontend/src/` and fails CI if any production file outside `CentralRouteManager.tsx` and `cache.ts` SETs one of the routing-owned reactive vars. If you've come to this section because that test fired, either route your write through the URL via `navigationUtils` helpers or — if you genuinely have an auth-driven (not URL-driven) case like `ProfileRedirect` — discuss adding the file to the explicit allowlist.
 
 ### Design Decisions
 
@@ -176,7 +175,12 @@ graph TB
 | `/c/:userIdent/:corpusIdent`           | `/c/john/my-corpus`           | CorpusLandingRoute   | Phase 1: Fetch corpus      |
 | `/d/:userIdent/:docIdent`              | `/d/john/my-document`         | DocumentLandingRoute | Phase 1: Fetch document    |
 | `/d/:userIdent/:corpusIdent/:docIdent` | `/d/john/corpus/doc`          | DocumentLandingRoute | Phase 1: Fetch both        |
-| `/e/:userIdent/:extractIdent`          | `/e/john/extract-123`         | ExtractLandingRoute  | Phase 1: Fetch extract     |
+| `/e/:userIdent/:extractIdent`          | `/e/john/extract-123`         | ExtractDetailRoute   | Phase 1: Fetch extract     |
+| `/extracts/:extractId`                 | `/extracts/extract-123`       | ExtractDetailRoute   | Phase 1: Fetch extract     |
+| `/users/:slug`                         | `/users/jane`                 | UserProfileRoute     | Phase 1: Fetch user        |
+| `/label_sets/:labelsetId`              | `/label_sets/labelset-1`      | LabelSetLandingRoute | Phase 1: Fetch labelset    |
+
+> `/profile` is handled by `ProfileRedirect`, a small auth-driven `<Navigate>` component that redirects to `/users/<current-user-slug>`. It lives outside CentralRouteManager because it is driven by auth state (`backendUserObj`), not URL state — once it redirects, Phase 1 takes over for the canonical `/users/:slug` path.
 
 ### Browse Routes (Query Params Only)
 
@@ -1328,25 +1332,32 @@ window.DEBUG_ROUTING = true
 
 ### Critical Rules (MUST Follow)
 
-**🚨 RULE #1: NEVER Set `openedCorpus`, `openedDocument`, or `openedExtract` Outside CentralRouteManager**
+**🚨 RULE #1: NEVER Set Routing-Owned Reactive Vars Outside CentralRouteManager**
+
+The full set: `openedCorpus`, `openedDocument`, `openedExtract`, `openedThread`, `openedLabelset`, `openedUser`.
 
 ```typescript
 // ❌ NEVER DO THIS (anywhere except CentralRouteManager.tsx):
 openedCorpus(someCorpus);
 openedDocument(someDocument);
 openedExtract(someExtract);
+openedUser(someUser);
 openedCorpus(null);
 openedDocument(null);
 openedExtract(null);
+openedUser(null);
 
 // ✅ ALWAYS DO THIS:
 const corpus = useReactiveVar(openedCorpus);  // Read only
 const document = useReactiveVar(openedDocument);  // Read only
 const extract = useReactiveVar(openedExtract);  // Read only
+const user = useReactiveVar(openedUser);  // Read only
 ```
 
 **Why?** Setting these vars from multiple places creates competing state updates, infinite loops, and route jittering. We learned this the hard way when `CorpusDocumentCards.tsx` was fetching corpus data and setting `openedCorpus()`, creating an infinite loop with `CentralRouteManager`.
 
+**CI enforcement:** `frontend/src/routing/__tests__/centralRouteDiscipline.test.ts` runs in the standard Vitest suite and fails if any production file outside the manager and `cache.ts` SETs one of these vars. Adding to its allowlist requires the same scrutiny as adding to RULE #1.
+
 **🚨 RULE #2: NEVER Fetch Entities in Route Components**
 
 ```typescript
@@ -1387,6 +1398,7 @@ const gotoHome = () => {
   openedCorpus(null);  // VIOLATION!
   openedDocument(null);  // VIOLATION!
   openedExtract(null);  // VIOLATION!
+  openedUser(null);  // VIOLATION!
   navigate("/corpuses");
 };
 
@@ -1397,7 +1409,7 @@ const gotoHome = () => {
 };
 ```
 
-**Why?** CentralRouteManager watches the URL and automatically clears `openedCorpus`/`openedDocument`/`openedExtract` when navigating to browse routes. Manual clearing creates race conditions.
+**Why?** CentralRouteManager watches the URL and automatically clears all entity vars when navigating to a browse route. Manual clearing creates race conditions.
 
 ### DO ✅
 

frontend/src/App.tsx

@@ -84,6 +84,7 @@ import { NotFound } from "./components/routes/NotFound";
 import { CorpusLandingRoute } from "./components/routes/CorpusLandingRoute";
 import { CorpusThreadRoute } from "./components/routes/CorpusThreadRoute";
 import { UserProfileRoute } from "./components/routes/UserProfileRoute";
+import { ProfileRedirect } from "./components/routes/ProfileRedirect";
 import { LeaderboardRoute } from "./components/routes/LeaderboardRoute";
 import { GlobalDiscussionsRoute } from "./components/routes/GlobalDiscussionsRoute";
 import { ThreadSearchRoute } from "./views/ThreadSearchRoute";
@@ -490,7 +491,7 @@ export const App = () => {
                   <Route path="/threads" element={<ThreadSearchRoute />} />
 
                   {/* User Profile Routes (Issue #611) */}
-                  <Route path="/profile" element={<UserProfileRoute />} />
+                  <Route path="/profile" element={<ProfileRedirect />} />
                   <Route path="/users/:slug" element={<UserProfileRoute />} />
                   {/* Convenience redirect for badge notifications (Issue #637) */}
                   <Route

frontend/src/components/labelsets/LabelSetDetailPage.tsx

@@ -259,7 +259,7 @@ export const LabelSetDetailPage: React.FC<LabelSetDetailPageProps> = ({
     if (onClose) {
       onClose();
     } else {
-      openedLabelset(null);
+      // CentralRouteManager Phase 1 clears openedLabelset on browse routes.
       navigate("/label_sets");
     }
   };
@@ -453,7 +453,7 @@ export const LabelSetDetailPage: React.FC<LabelSetDetailPageProps> = ({
       .then((result) => {
         if (result.data?.deleteLabelset?.ok) {
           toast.success("Label set deleted successfully");
-          openedLabelset(null);
+          // CentralRouteManager Phase 1 clears openedLabelset on browse routes.
           navigate("/label_sets");
         } else {
           toast.error(

frontend/src/components/routes/ExtractDetailRoute.tsx

@@ -1,60 +1,28 @@
-import React, { useEffect } from "react";
-import { useParams } from "react-router-dom";
-import { useQuery, useReactiveVar } from "@apollo/client";
+import React from "react";
+import { useReactiveVar } from "@apollo/client";
 import { MetaTags } from "../seo/MetaTags";
 import { ModernLoadingDisplay } from "../widgets/ModernLoadingDisplay";
 import { ModernErrorDisplay } from "../widgets/ModernErrorDisplay";
 import { ErrorBoundary } from "../widgets/ErrorBoundary";
-import { openedExtract } from "../../graphql/cache";
+import { openedExtract, routeLoading, routeError } from "../../graphql/cache";
 import { ExtractDetail } from "../../views/ExtractDetail";
-import {
-  RESOLVE_EXTRACT_BY_ID,
-  ResolveExtractByIdOutput,
-  ResolveExtractByIdInput,
-} from "../../graphql/queries";
 
 /**
- * ExtractDetailRoute - Handles the /extracts/:extractId route
+ * ExtractDetailRoute - Renders the extract detail view for /extracts/:extractId
+ * and /e/:userIdent/:extractIdent.
  *
- * This is the new route-based pattern for viewing extract details,
- * replacing the modal-based EditExtractModal.
- *
- * Route pattern: /extracts/:extractId
+ * URL parsing, GraphQL resolution, and reactive-var population are owned by
+ * CentralRouteManager. This component reads the resolved state and renders.
  */
 export const ExtractDetailRoute: React.FC = () => {
-  const { extractId } = useParams<{ extractId: string }>();
-
-  // Check if we already have the extract from reactive var
-  const existingExtract = useReactiveVar(openedExtract);
-
-  // Query to resolve extract by ID if not already loaded
-  const { loading, error, data } = useQuery<
-    ResolveExtractByIdOutput,
-    ResolveExtractByIdInput
-  >(RESOLVE_EXTRACT_BY_ID, {
-    variables: { extractId: extractId ?? "" },
-    skip: !extractId || existingExtract?.id === extractId,
-    fetchPolicy: "network-only",
-  });
-
-  // Set the opened extract when query completes
-  useEffect(() => {
-    if (data?.extract) {
-      openedExtract(data.extract);
-    }
-  }, [data]);
+  const extract = useReactiveVar(openedExtract);
+  const loading = useReactiveVar(routeLoading);
+  const error = useReactiveVar(routeError);
 
-  // Handle missing extractId
-  if (!extractId) {
-    return <ModernErrorDisplay type="extract" error="No extract ID provided" />;
-  }
-
-  // Loading state
-  if (loading && !existingExtract) {
+  if (loading && !extract) {
     return <ModernLoadingDisplay type="extract" size="large" />;
   }
 
-  // Error state
   if (error) {
     return (
       <ModernErrorDisplay
@@ -64,20 +32,15 @@ export const ExtractDetailRoute: React.FC = () => {
     );
   }
 
-  // Get the extract to display
-  const extract =
-    existingExtract?.id === extractId ? existingExtract : data?.extract;
-
-  // Not found state
-  if (!extract && !loading) {
+  if (!extract) {
     return <ModernErrorDisplay type="extract" error="Extract not found" />;
   }
 
   return (
     <ErrorBoundary>
       <MetaTags
-        title={extract?.name || "Extract"}
-        description={`Extract: ${extract?.name}`}
+        title={extract.name || "Extract"}
+        description={`Extract: ${extract.name}`}
         entity={extract}
         entityType="extract"
       />

frontend/src/components/routes/LabelSetLandingRoute.tsx

@@ -31,9 +31,9 @@ export const LabelSetLandingRoute: React.FC = () => {
     hasError: !!error,
   });
 
-  // Handle close by navigating back to list
+  // Handle close by navigating back to list. CentralRouteManager Phase 1
+  // clears openedLabelset when the new path resolves to a browse route.
   const handleClose = () => {
-    openedLabelset(null);
     navigate("/label_sets");
   };
 

frontend/src/components/routes/ProfileRedirect.tsx

@@ -0,0 +1,43 @@
+/**
+ * ProfileRedirect - Resolves /profile to the current user's canonical
+ * /users/:slug URL.
+ *
+ * This is auth-state-driven (not URL-state-driven), so it lives outside
+ * the CentralRouteManager entity-resolution path. It reads backendUserObj
+ * and renders a React Router <Navigate> to the canonical profile URL,
+ * which then triggers CentralRouteManager Phase 1 user resolution.
+ *
+ * Anonymous visitors are sent to /login.
+ */
+
+import React from "react";
+import { Navigate } from "react-router-dom";
+import { useReactiveVar } from "@apollo/client";
+import {
+  backendUserObj,
+  authStatusVar,
+  authInitCompleteVar,
+} from "../../graphql/cache";
+import { ModernLoadingDisplay } from "../widgets/ModernLoadingDisplay";
+
+export const ProfileRedirect: React.FC = () => {
+  const currentUser = useReactiveVar(backendUserObj);
+  const authStatus = useReactiveVar(authStatusVar);
+  const authInitComplete = useReactiveVar(authInitCompleteVar);
+
+  // While Auth0 is still resolving the session, backendUserObj is null even
+  // for an authenticated visitor. Redirecting before this settles would
+  // briefly send an authenticated user to /login. Hold the render until the
+  // auth pipeline (token + cache reset) signals it's done.
+  if (authStatus === "LOADING" || !authInitComplete) {
+    return <ModernLoadingDisplay type="default" message="Resolving profile…" />;
+  }
+
+  if (!currentUser?.slug) {
+    return <Navigate to="/login" replace />;
+  }
+
+  return <Navigate to={`/users/${currentUser.slug}`} replace />;
+};
+
+export default ProfileRedirect;