From 75aaaf1ed657327b7f86ebe44cb5195f2cf445c6 Mon Sep 17 00:00:00 2001 From: adamflagg <115844481+adamflagg@users.noreply.github.com> Date: Thu, 21 May 2026 13:00:26 -0700 Subject: [PATCH 1/3] refactor(frontend): readonly on graph cache/API types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §3 #8 first slice. Adds `readonly` to every field of the hand-written `types/graph.ts` interfaces (GraphNode, GraphEdge, GraphMetrics, CrossScopeEdge, GraphData) and `readonly T[]` on the array fields. Why: GraphData is fetched once, cached in GraphCacheService, and read by ~5 graph components + the layout worker. A stray in-place mutation (.push/.sort/field reassignment) on the shared cached instance would corrupt every subscriber; readonly makes that a compile error. Pure type-level — no runtime change. Node dragging/layout repositioning is unaffected: positions live on Cytoscape's NodeSingular model, and GraphData.layout_positions is read-only seed data (only written `= {}` in test mocks). Verified zero tsc cascade across all 5 consumers / 42 references — the codebase already copies-before-mutating, so nothing fights readonly. Generated type files (pocketbase-types.ts, api-generated/) deliberately skipped; app-types.ts composites are the remaining slice of this row. Also reconciles backlog §3c against git: marks #1587/#1588/#1589/#1591 shipped, retires the error-cause row (3a#5) as a false positive, adds Retired rows for error-cause + the satisfies tuple-widening trap, and corrects the readonly count (24 → 29). Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/reference/modernization-backlog.md | 16 ++-- frontend/src/types/graph.ts | 99 +++++++++++++------------ 2 files changed, 62 insertions(+), 53 deletions(-) diff --git a/docs/reference/modernization-backlog.md b/docs/reference/modernization-backlog.md index 4ca1739bf..50fd67de4 100644 --- a/docs/reference/modernization-backlog.md +++ b/docs/reference/modernization-backlog.md @@ -194,7 +194,7 @@ Survey scope: Go 1.18 through 1.26. Re-run Part A on a 1.27+ toolchain bump. | 5 | `catch (e) { throw new Error(msg) }` | `throw new Error(msg, { cause: e })` | ES2015 | ES2022 (already in lib) | **HIGH** (stack-trace fidelity) | catch-and-wrap subset of 93 `catch` blocks (`services/*`, hooks); only `useCamperMovement.ts` chains today | scope to catch-rethrow sites (NOT the 138 bare `throw`) | | 6 | `xs.reduce>((acc, x) => { acc[k].push(x) }, {})` | `Object.groupBy(xs, x => k)` | ES5 | ES2024 | LOW–MED | `components/LockGroupPanel.tsx:339`, `contexts/LockGroupContext.tsx:165` (near-duplicates) | 2 | | 7 | ad-hoc `useQuery({queryKey, queryFn})` + `enabled: ` | `queryOptions(...)` shared defs + `skipToken` | RQ v5 | RQ v5 (in use) | **HIGH** (type-safe shared defs; `skipToken` replaces conditional `enabled`) | 103 `useQuery`/55 files, 112 `enabled:` gates, 4 `useQueries` (no `combine`) | 50+ | -| 8 | mutable `interface`/`type` for cache/store/API shapes | `readonly` fields / `Readonly<>` | TS | TS | MEDIUM (prevents accidental mutation) | ≈1,758 type defs (735 interface + 1,023 alias); 24 `readonly` today | targeted (persisted/cached shapes only) | +| 8 | mutable `interface`/`type` for cache/store/API shapes | `readonly` fields / `Readonly<>` | TS | TS | MEDIUM (prevents accidental mutation) | ≈1,758 type defs (735 interface + 1,023 alias); **29** `readonly` today (was 24 — counts decay). **Skip generated** (`types/pocketbase-types.ts`, `types/api-generated/`) — hand-edits lost on regen. First slice: `types/graph.ts` (`GraphData`/`Node`/`Edge`, cached in `GraphCacheService`); `types/app-types.ts` composites the remaining slice | targeted (persisted/cached shapes only); 0 in-place mutations exist today → forward-looking insurance, not a bug fix | | 9 | untyped / `as Type` config & route/feature maps | `… satisfies T` | TS 4.9 | TS 4.9 (in use) | MEDIUM | 4 files use it (`VelocityPage`, `CancellationVelocityPage`, `services/debug.ts`); 208 `as const` | targeted | **Two-version note:** only rows 2 and 6 carry a *compile* gate — the ES2022 `lib` doesn't expose `toSorted`/`Object.groupBy` types (hence row 1). Every other rewrite is already accepted by the current toolchain, so it's stylistic/robustness with no compile-error pressure. @@ -227,6 +227,8 @@ Survey scope: Go 1.18 through 1.26. Re-run Part A on a 1.27+ toolchain bump. | `[...set].sort()` / `[...map.entries()].sort()` → `toSorted` | 10 sites (`retentionTransforms.ts` ×2, `SessionBunkHeatmap.tsx` ×2, `useVelocityChartData.ts` ×2, `useStaffRetentionData.ts`, `PdfExport/familyRows.ts`, `PostValidationResultsModal.tsx`, `PdfExport/BunkPlanReport.tsx`) | When the spread *materializes* a `Set`/`Map`/iterator (which have no `.sort()`/`.toSorted()`), the spread is **load-bearing**, not a defensive copy of an array. `[...x].toSorted()` allocates a second throwaway array; `[...x].sort()` sorts the first in place. `toSorted` is only a win over a *named Array* (`[...arr].sort()` → `arr.toSorted()`). **Lesson (#1587): grep `[\.\.\.IDENT]` is not enough — confirm IDENT is an Array, not a Set/Map/iterator; also exclude array-literal-append spreads like `[...prev, n].sort()`.** | | `use()` "eliminates throw-on-missing-provider wrapper" | prior HIGH row | `use(Context)` doesn't auto-throw or remove the null-guard; it only allows conditional reads. **Lesson: verify the feature's real semantics, not its reputation.** | | inflated counts | "100+ useContext", "~1,800 as const", "240 type defs", "only 2 lazy" | actual 13 / 208 / ≈1,758 / ≈50-lazy. **Lesson: counts decay — re-grep every pass (carried from §1/§2).** | +| error `cause` chaining (3a#5) | prior **HIGH** row #6 — "catch-and-wrap subset of 93 catch blocks" | All 10 wrapping `catch` blocks re-throw the original verbatim (`throw error`); the lone genuine wrap (`useCamperMovement.ts:297`) **already** passes `{ cause }`. The audit conflated "93 `catch` blocks" with "catch-and-wrap-*without*-cause" — zero of the latter exist. **Lesson: `catch` count ≠ wrap-rethrow count; grep for `throw new Error(` *inside* a catch that drops the caught variable, not bare `catch (`.** | +| `satisfies` on array/tuple literals (3a#9) | 2 sites that wouldn't flip during #1591 | `[lat, lng] as LatLng` / `[...] as VelocityViewMode[]` can't become `… satisfies T`: a `satisfies`-checked array literal **widens** to `number[]`/`string[]`, losing the tuple / literal-union element type the `as` cast asserts. `satisfies` only helps object literals that already structurally conform. Separately spun off **issue #1590** — two `as` casts masking `exactOptionalPropertyTypes` mismatches (can't flip until the underlying types are fixed). **Lesson: `satisfies` ≠ `as` for arrays/tuples — it can't narrow literal-union or tuple element types.** | ### 3c. Ranked execution order @@ -234,12 +236,12 @@ Survey scope: Go 1.18 through 1.26. Re-run Part A on a 1.27+ toolchain bump. |---|---|---|---| | 1 | `✓ shipped #1585` | 3a#1 | `build(frontend): bump tsconfig target+lib to ES2024` (unblocks #3, #5) | | 2 | `✓ shipped #1586` | 3a#3 | `refactor(frontend): arr[len-1] → arr.at(-1)` (27 sites, no prereq) | -| 3 | `PR open #1587` | 3a#2 | `refactor(frontend): [...].sort() → toSorted` (30 true-win sites / 22 files; Set/iterator spreads skipped — see Retired) | -| 4 | `next` | 3a#4 | `refactor(frontend): → shorthand` (18 sites) | -| 5 | | 3a#6 | `refactor(frontend): Object.groupBy for LockGroup membersByGroup` (2 sites; needs #1) | -| 6 | | 3a#5 | `refactor(frontend): error cause chaining on catch-rethrow` (behavioral — TDD per Rule 3) | -| 7 | | 3a#9 | `refactor(frontend): satisfies on config/route maps` (targeted) | -| 8 | | 3a#8 | `refactor(frontend): readonly on cache/store/API types` (type-level, large surface) | +| 3 | `✓ shipped #1587` | 3a#2 | `refactor(frontend): [...].sort() → toSorted` (30 true-win sites / 22 files; Set/iterator spreads skipped — see Retired) | +| 4 | `✓ shipped #1588` | 3a#4 | `refactor(frontend): → shorthand` (18 sites) | +| 5 | `✓ shipped #1589` | 3a#6 | `refactor(frontend): Object.groupBy for LockGroup membersByGroup` (2 sites; needs #1) | +| 6 | `skipped (retired — FP)` | 3a#5 | `refactor(frontend): error cause chaining on catch-rethrow` — retired, see Retired table (0 wrap-rethrow-without-cause sites) | +| 7 | `✓ shipped #1591` | 3a#9 | `refactor(frontend): satisfies on config/route maps` (targeted) | +| 8 | `next` | 3a#8 | `refactor(frontend): readonly on cache/store/API types` (type-level, large surface; split per-shape — `graph.ts` slice first, `app-types.ts` follow-up) | | 9 | | 3a#7 | `refactor(frontend): React Query queryOptions + skipToken` (HIGH value, largest/most semantic — split as needed) | Ranking is easiest→hardest with the unblocker (#1) first per Part B's tie-breaker. The HIGH-impact-but-large rows (error cause, React Query) sit later because they carry test/semantic surface, not because they're low value. diff --git a/frontend/src/types/graph.ts b/frontend/src/types/graph.ts index 0cde9e671..87bbc9255 100644 --- a/frontend/src/types/graph.ts +++ b/frontend/src/types/graph.ts @@ -1,81 +1,88 @@ /** * Types for social network graph data + * + * These shapes are fetched once and cached in `GraphCacheService`, then read by + * multiple subscribers (graph components, the layout worker, the bubble + * renderer). `readonly` makes the shared-cache contract explicit at the type + * level: a stray in-place mutation (`.push`/`.sort`/field reassignment) on the + * cached instance would corrupt every consumer, so the compiler forbids it. + * Defensive copies (`[...nodes]`, `.map(...)`) are unaffected. */ export interface GraphNode { - id: number - name: string - grade: number | null - bunk_cm_id: number | null - centrality: number - clustering: number - community: number | null - satisfaction_status?: 'satisfied' | 'unsatisfied' | 'no_requests' + readonly id: number + readonly name: string + readonly grade: number | null + readonly bunk_cm_id: number | null + readonly centrality: number + readonly clustering: number + readonly community: number | null + readonly satisfaction_status?: 'satisfied' | 'unsatisfied' | 'no_requests' // Stage 2 parent-paramount split. Both fields drive 3b's parent/staff edge // filter checkbox; in 3a, only parent_satisfaction_status is rendered as the // node border color. - parent_satisfaction_status?: 'satisfied' | 'unsatisfied' | 'no_requests' - staff_satisfaction_status?: 'satisfied' | 'unsatisfied' | 'no_requests' + readonly parent_satisfaction_status?: 'satisfied' | 'unsatisfied' | 'no_requests' + readonly staff_satisfaction_status?: 'satisfied' | 'unsatisfied' | 'no_requests' // Legacy fields that may still be referenced - age?: number - sex?: string - degree_centrality?: number - betweenness_centrality?: number - isolated?: boolean + readonly age?: number + readonly sex?: string + readonly degree_centrality?: number + readonly betweenness_centrality?: number + readonly isolated?: boolean } export interface GraphEdge { - source: number - target: number - weight: number + readonly source: number + readonly target: number + readonly weight: number /** Edge type. Always 'request' — the API only emits request edges. */ - edge_type: string - reciprocal: boolean - request_type?: string // 'bunk_with' | 'not_bunk_with' for edge_type='request' edges - confidence?: number // AI confidence score for request edges - metadata?: Record // Additional edge metadata + readonly edge_type: string + readonly reciprocal: boolean + readonly request_type?: string // 'bunk_with' | 'not_bunk_with' for edge_type='request' edges + readonly confidence?: number // AI confidence score for request edges + readonly metadata?: Record // Additional edge metadata // Legacy fields that may still be referenced - is_reciprocal?: boolean + readonly is_reciprocal?: boolean } export interface GraphMetrics { - density: number - average_clustering: number - number_of_components: number - average_degree: number - [key: string]: number + readonly density: number + readonly average_clustering: number + readonly number_of_components: number + readonly average_degree: number + readonly [key: string]: number } /** Shape of a cross-scope edge as serialized by the Python CrossScopeEdge * Pydantic model. Pydantic emits `null` for absent Optional fields (not * `undefined`), so nullable required fields are typed `T | null`. */ export interface CrossScopeEdge { - source: number - target: number - edge_type: string - weight: number - request_type: string | null - confidence: number | null - reciprocal: boolean - cross_scope: true + readonly source: number + readonly target: number + readonly edge_type: string + readonly weight: number + readonly request_type: string | null + readonly confidence: number | null + readonly reciprocal: boolean + readonly cross_scope: true } export interface GraphData { - nodes: GraphNode[] - edges: GraphEdge[] - metrics: GraphMetrics + readonly nodes: readonly GraphNode[] + readonly edges: readonly GraphEdge[] + readonly metrics: GraphMetrics // Session-level only. The bunk-graph endpoint (BunkGraphResponse) does not // emit this field, and the same GraphCacheService stores both shapes. - communities?: Record - warnings?: string[] - layout_positions?: Record - edge_type_counts?: Record + readonly communities?: Record + readonly warnings?: readonly string[] + readonly layout_positions?: Record + readonly edge_type_counts?: Record /** Edges crossing the scope boundary when ?cross_scope=true. Frontend * renders these as ghosted to show context without polluting the layout. * Shape mirrors the Python CrossScopeEdge Pydantic model. */ - cross_scope_edges?: CrossScopeEdge[] + readonly cross_scope_edges?: readonly CrossScopeEdge[] /** Out-of-scope endpoints of cross_scope_edges. Frontend renders these as * ghosted-but-clickable nodes so users can click through to a potential * connection, while the layout still treats the in-scope set as the focus. */ - cross_scope_nodes?: GraphNode[] + readonly cross_scope_nodes?: readonly GraphNode[] } From 17d7ea3e9f5e1e9462230999d7b68db52d1702c7 Mon Sep 17 00:00:00 2001 From: adamflagg <115844481+adamflagg@users.noreply.github.com> Date: Thu, 21 May 2026 13:01:28 -0700 Subject: [PATCH 2/3] =?UTF-8?q?docs(docs):=20mark=20=C2=A73c=20#8=20PR=20o?= =?UTF-8?q?pen=20#1592?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/reference/modernization-backlog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/modernization-backlog.md b/docs/reference/modernization-backlog.md index 50fd67de4..4a0b82670 100644 --- a/docs/reference/modernization-backlog.md +++ b/docs/reference/modernization-backlog.md @@ -241,7 +241,7 @@ Survey scope: Go 1.18 through 1.26. Re-run Part A on a 1.27+ toolchain bump. | 5 | `✓ shipped #1589` | 3a#6 | `refactor(frontend): Object.groupBy for LockGroup membersByGroup` (2 sites; needs #1) | | 6 | `skipped (retired — FP)` | 3a#5 | `refactor(frontend): error cause chaining on catch-rethrow` — retired, see Retired table (0 wrap-rethrow-without-cause sites) | | 7 | `✓ shipped #1591` | 3a#9 | `refactor(frontend): satisfies on config/route maps` (targeted) | -| 8 | `next` | 3a#8 | `refactor(frontend): readonly on cache/store/API types` (type-level, large surface; split per-shape — `graph.ts` slice first, `app-types.ts` follow-up) | +| 8 | `PR open #1592` (graph.ts slice) | 3a#8 | `refactor(frontend): readonly on cache/store/API types` (type-level, large surface; split per-shape — `graph.ts` slice = #1592, `app-types.ts` composites still open) | | 9 | | 3a#7 | `refactor(frontend): React Query queryOptions + skipToken` (HIGH value, largest/most semantic — split as needed) | Ranking is easiest→hardest with the unblocker (#1) first per Part B's tie-breaker. The HIGH-impact-but-large rows (error cause, React Query) sit later because they carry test/semantic surface, not because they're low value. From 797fa2c3f3ee4beec0571ba730a957d4c2297e99 Mon Sep 17 00:00:00 2001 From: adamflagg <115844481+adamflagg@users.noreply.github.com> Date: Thu, 21 May 2026 13:41:33 -0700 Subject: [PATCH 3/3] refactor(frontend): seal nested Record fields in graph types as Readonly Wrap the four mutable Record-typed fields (GraphEdge.metadata, GraphData.communities/layout_positions/edge_type_counts) in Readonly<> so the immutability contract covers nested map contents, not just the field reference. Previously `data.edge_type_counts!['x'] = 9` still type-checked, contradicting the header doc comment's "the compiler forbids it" claim. Addresses CodeRabbit review on #1592. Type-only; type-check green across all 5 consumers / 42 references. Co-Authored-By: Claude Opus 4.7 (1M context) --- frontend/src/types/graph.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frontend/src/types/graph.ts b/frontend/src/types/graph.ts index 87bbc9255..84d67ee23 100644 --- a/frontend/src/types/graph.ts +++ b/frontend/src/types/graph.ts @@ -40,7 +40,7 @@ export interface GraphEdge { readonly reciprocal: boolean readonly request_type?: string // 'bunk_with' | 'not_bunk_with' for edge_type='request' edges readonly confidence?: number // AI confidence score for request edges - readonly metadata?: Record // Additional edge metadata + readonly metadata?: Readonly> // Additional edge metadata // Legacy fields that may still be referenced readonly is_reciprocal?: boolean } @@ -73,10 +73,10 @@ export interface GraphData { readonly metrics: GraphMetrics // Session-level only. The bunk-graph endpoint (BunkGraphResponse) does not // emit this field, and the same GraphCacheService stores both shapes. - readonly communities?: Record + readonly communities?: Readonly> readonly warnings?: readonly string[] - readonly layout_positions?: Record - readonly edge_type_counts?: Record + readonly layout_positions?: Readonly> + readonly edge_type_counts?: Readonly> /** Edges crossing the scope boundary when ?cross_scope=true. Frontend * renders these as ghosted to show context without polluting the layout. * Shape mirrors the Python CrossScopeEdge Pydantic model. */