Skip to content

Commit e396304

Browse files
committed
Document /users/:slug and /extracts/:extractId in routing guide and CHANGELOG
Update docs/frontend/routing_system.md: - Route-pattern table now lists /users/:slug, /extracts/:extractId, and /label_sets/:labelsetId. Adds a callout that /profile is handled by ProfileRedirect (auth-driven, lives outside the manager by design). - "ONLY CentralRouteManager may SET" list extended to include openedThread, openedLabelset, and openedUser. - All four critical RULE blocks updated for the same set; references the new centralRouteDiscipline.test.ts as the CI enforcement mechanism so future contributors know what failed and why. Add a CHANGELOG entry under [Unreleased] / Changed describing the route-resolution centralization, the discipline regression test, the ProfileRedirect split, and the side-fix for the pre-existing CamlDirectiveRenderer typecheck failure.
1 parent 68e2a29 commit e396304

2 files changed

Lines changed: 26 additions & 4 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2828

2929
### Changed
3030

31+
- **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).
32+
- 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.
33+
- 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.
34+
- **Pre-existing `tsc` failure on `frontend/src/components/corpuses/caml/CamlDirectiveRenderer.tsx`** resolved as part of the same PR: the installed `@os-legal/caml-react@0.0.1` d.ts files lack the `resolveImageSrc` prop that the declared semver (`^0.1.0`) source already exposes, so `tsc --noEmit` was failing on `main`. Added a narrow local type widening with a comment so `tsc` compiles cleanly until the lockfile is refreshed; runtime behavior is unchanged.
3135
- **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.
3236
- **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.
3337

docs/frontend/routing_system.md

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ The OpenContracts routing system follows a **centralized architecture** where **
3636
- `openedCorpus()`
3737
- `openedDocument()`
3838
- `openedExtract()`
39+
- `openedThread()`
40+
- `openedLabelset()`
41+
- `openedUser()`
3942

4043
**URL-Driven State (set by Phase 2, watched by Phase 4):**
4144
- `selectedAnnotationIds()`
@@ -62,7 +65,9 @@ This is non-negotiable. Violations cause infinite loops, route jittering, compet
6265
- `CorpusBreadcrumbs.tsx` (manual clearing)
6366
- `Corpuses.tsx` (3 violations)
6467

65-
**If you find yourself writing `openedCorpus(someValue)`, `openedDocument(someValue)`, or `openedExtract(someValue)` anywhere except `CentralRouteManager.tsx`, STOP. You are introducing a bug.**
68+
**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.**
69+
70+
> 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.
6671
6772
### Design Decisions
6873

@@ -176,7 +181,12 @@ graph TB
176181
| `/c/:userIdent/:corpusIdent` | `/c/john/my-corpus` | CorpusLandingRoute | Phase 1: Fetch corpus |
177182
| `/d/:userIdent/:docIdent` | `/d/john/my-document` | DocumentLandingRoute | Phase 1: Fetch document |
178183
| `/d/:userIdent/:corpusIdent/:docIdent` | `/d/john/corpus/doc` | DocumentLandingRoute | Phase 1: Fetch both |
179-
| `/e/:userIdent/:extractIdent` | `/e/john/extract-123` | ExtractLandingRoute | Phase 1: Fetch extract |
184+
| `/e/:userIdent/:extractIdent` | `/e/john/extract-123` | ExtractDetailRoute | Phase 1: Fetch extract |
185+
| `/extracts/:extractId` | `/extracts/extract-123` | ExtractDetailRoute | Phase 1: Fetch extract |
186+
| `/users/:slug` | `/users/jane` | UserProfileRoute | Phase 1: Fetch user |
187+
| `/label_sets/:labelsetId` | `/label_sets/labelset-1` | LabelSetLandingRoute | Phase 1: Fetch labelset |
188+
189+
> `/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.
180190
181191
### Browse Routes (Query Params Only)
182192

@@ -1328,25 +1338,32 @@ window.DEBUG_ROUTING = true
13281338
13291339
### Critical Rules (MUST Follow)
13301340
1331-
**🚨 RULE #1: NEVER Set `openedCorpus`, `openedDocument`, or `openedExtract` Outside CentralRouteManager**
1341+
**🚨 RULE #1: NEVER Set Routing-Owned Reactive Vars Outside CentralRouteManager**
1342+
1343+
The full set: `openedCorpus`, `openedDocument`, `openedExtract`, `openedThread`, `openedLabelset`, `openedUser`.
13321344
13331345
```typescript
13341346
// ❌ NEVER DO THIS (anywhere except CentralRouteManager.tsx):
13351347
openedCorpus(someCorpus);
13361348
openedDocument(someDocument);
13371349
openedExtract(someExtract);
1350+
openedUser(someUser);
13381351
openedCorpus(null);
13391352
openedDocument(null);
13401353
openedExtract(null);
1354+
openedUser(null);
13411355

13421356
// ✅ ALWAYS DO THIS:
13431357
const corpus = useReactiveVar(openedCorpus); // Read only
13441358
const document = useReactiveVar(openedDocument); // Read only
13451359
const extract = useReactiveVar(openedExtract); // Read only
1360+
const user = useReactiveVar(openedUser); // Read only
13461361
```
13471362
13481363
**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`.
13491364
1365+
**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.
1366+
13501367
**🚨 RULE #2: NEVER Fetch Entities in Route Components**
13511368
13521369
```typescript
@@ -1387,6 +1404,7 @@ const gotoHome = () => {
13871404
openedCorpus(null); // VIOLATION!
13881405
openedDocument(null); // VIOLATION!
13891406
openedExtract(null); // VIOLATION!
1407+
openedUser(null); // VIOLATION!
13901408
navigate("/corpuses");
13911409
};
13921410

@@ -1397,7 +1415,7 @@ const gotoHome = () => {
13971415
};
13981416
```
13991417
1400-
**Why?** CentralRouteManager watches the URL and automatically clears `openedCorpus`/`openedDocument`/`openedExtract` when navigating to browse routes. Manual clearing creates race conditions.
1418+
**Why?** CentralRouteManager watches the URL and automatically clears all entity vars when navigating to a browse route. Manual clearing creates race conditions.
14011419
14021420
### DO ✅
14031421

0 commit comments

Comments
 (0)