refactor(frontend): readonly on graph cache/API types#1592
Merged
Conversation
§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) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGraph type definitions are made immutable by adding ChangesGraph type readonly enforcement and modernization tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/types/graph.ts`:
- Line 43: The field types using mutable Record maps (e.g., the metadata
property declared as readonly metadata?: Record<string, unknown>) allow callers
to mutate cached objects via index assignment; replace those mutable maps with
readonly equivalents by changing Record<string, unknown> to
Readonly<Record<string, unknown>> (and similarly for any other occurrences in
this file, including the fields around lines 76-79) so the type system enforces
immutability of nested map entries for functions/types like metadata and the
other map-typed properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1163cfd-3b93-43b5-bf3a-ea350b84ac8c
📒 Files selected for processing (2)
docs/reference/modernization-backlog.mdfrontend/src/types/graph.ts
This was referenced May 21, 2026
Closed
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) <noreply@anthropic.com>
adamflagg
added a commit
that referenced
this pull request
May 21, 2026
Backlog row **§3 #8**, second/final slice — `types/app-types.ts` (graph.ts shipped in #1592). This closes out #8. ## What it does Adds `readonly` to every field of the hand-written app-types interfaces — `Camper`, `BunkRequest`, `Constraint`, `SolverRun`, `DragItem`, `BunkWithCampers` — with `readonly T[]` on array fields and `Readonly<Record<…>>` on map fields (matching the nested-Record pattern #1592 picked up from CodeRabbit). **Pure type-level — no runtime change.** The generated PB aliases (`Session`, `Bunk`, `SavedScenario`) are left mutable — their immutability belongs in codegen, not a hand-edit. ## Why `Camper`/`BunkRequest` are the domain shapes that live in the React Query cache and flow through the board, panels, and drag-and-drop. A stray in-place mutation on a shared cached instance would corrupt every subscriber; `readonly` makes that a compile error. ## Cascade — measured, tiny A full-readonly `tsc` probe surfaced **exactly 2** consumer sites (even with `Camper` used in 105 files), both the predicted `readonly T[]`→mutable-param class, neither a real mutation: | Site | Fix | |------|-----| | `csvExportHelpers.ts` `buildCamperRows(campers)` | widened param to `readonly Camper[]` — it only `filter`/`toSorted`/`map`s (all return fresh arrays) | | `RequestForm.tsx` `useState(constraint?.campers ?? [])` | copies into a mutable working copy `[...]`; source stays immutable | ## Verification - `npm run type-check` (both tsconfigs): green - `csvExportHelpers` vitest: 19/19 - full `lefthook run pre-push`: green Consistent with #1592's finding: zero in-place mutations of cached data exist today, so `readonly` is forward-looking insurance with near-zero cascade. Marks §3c #8 done (both slices); next live row is **#9** (React Query `queryOptions`/`skipToken`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed potential issue in request form where camper selections could inadvertently modify source data. * **Documentation** * Updated modernization backlog to reflect completion of type system improvements. * **Refactor** * Improved internal data immutability across core application types to enhance code reliability and prevent unintended data mutations. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/adamflagg/kindred/pull/1595?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
](https://app.coderabbit.ai/change-stack/adamflagg/kindred/pull/1595?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
Merged
3 tasks
adamflagg
added a commit
that referenced
this pull request
May 21, 2026
…70: #1593, #1594, #1590) (#1597) Closes #1593, closes #1594, closes #1590. Group 70 — type-safety follow-ups deferred out of the §3 frontend-modernization PRs (#1585–#1592). Pure type-only hardening; no runtime behavior change. Each fix makes a type honest so a load-bearing cast can be deleted or flipped to `satisfies`. ## Changes - **#1593** — widen `createGraphElements`'s four array params to `readonly` + narrow graph.ts `CrossScopeEdge.edge_type` to `'request'` (matches the generated type; the API only emits `'request'` cross-scope edges). All four `as Parameters<…>[N]` casts at `SocialNetworkGraph.tsx` are deleted. - **#1594** — seal the local `BunkGraphData.nodes`/`.edges` `readonly`. The `as unknown as BunkGraphData` stays (the cached `GraphData` structurally diverges — `bunk_cm_id`/`bunk_name`/`metrics`/`health_score`), but downstream read-only protection is restored. - **#1590** — make two literals honest under `exactOptionalPropertyTypes`: - `useCamperEnrollment`: flipping `as Camper` → `satisfies Camper` surfaced two masked violations — `assigned_bunk_cm_id` (`number | undefined` into optional `?: number`, now conditional-spread) and `expand.session`/`.assigned_bunk` (explicit `undefined` into `?: … | null`, now `?? null`). Also tightened a now-redundant `expand?.` optional chain. - `useSiblings`: `as SiblingWithEnrollment` → `satisfies`; conditional-spread the optional `session` key; typed `session_type` as the `CampSessionsSessionTypeOptions` enum; dropped the manual `s is SiblingWithEnrollment` predicate in favor of TS 6's inferred narrowing (the precise literal type is a subtype, satisfying the hook's return boundary). ## Test Plan - [x] `npm run type-check` passes (both `tsconfig.json` + `tsconfig.node.json`) — each cast was verified load-bearing (removed → tsc fails) before its supporting type change. - [x] `npm run lint` — 0 errors, no new warnings (the two introduced by the type-honesty change are fixed). - [x] `npx vitest run` — 4450 pass, 0 fail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved internal type safety by strengthening immutability constraints on data structures used for graph rendering and camper enrollment information. * Enhanced type accuracy for cross-scope edge data and session information to better align with backend contracts. * Simplified null-checking logic in enrollment and sibling data processing. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/adamflagg/kindred/pull/1597?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
](https://app.coderabbit.ai/change-stack/adamflagg/kindred/pull/1597?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backlog row §3 #8 (
docs/reference/modernization-backlog.md), first slice —types/graph.ts.What it does
Adds
readonlyto every field of the hand-written graph interfaces (GraphNode,GraphEdge,GraphMetrics,CrossScopeEdge,GraphData) andreadonly T[]on the array fields (nodes,edges,cross_scope_edges,cross_scope_nodes). Pure type-level — no runtime change.Why
GraphDatais fetched once, cached inGraphCacheService, and read by ~5 graph components plus the layout worker. A stray in-place mutation (.push/.sort/field reassignment) on the shared cached instance would silently corrupt every subscriber;readonlyturns that into a compile error. This is the clearest instance of the shared-cache bug-class in the frontend.Does NOT affect node movement
Dragging/layout repositioning operates on Cytoscape's own
NodeSingularmodel (node.position(pos)inSocialNetworkGraph.tsx,layoutWorkerGuards.ts), never ourGraphData.GraphData.layout_positionsis read-only seed data — only ever read in production (the lone writes are= {}in two test mocks).Verification
npm run type-check: zero tsc cascade across all 5 consumers / 42 references — the codebase already copies-before-mutating, so nothing fightsreadonly.lefthook run pre-push: green.Generated type files (
pocketbase-types.ts,api-generated/) deliberately skipped (hand-edits lost on regen).app-types.tscomposites are the remaining slice of this row.Also (bundled doc reconciliation)
Reconciles backlog §3c against git: marks #1587/#1588/#1589/#1591 shipped, retires the error-cause row (3a#5) as a false positive (0 wrap-rethrow-without-cause sites), adds Retired rows for error-cause + the
satisfiestuple-widening trap, and corrects thereadonlycount (24 → 29).🤖 Generated with Claude Code
Summary by CodeRabbit