Skip to content

refactor(frontend): readonly on app-types cache/API shapes#1595

Merged
adamflagg merged 1 commit into
mainfrom
feature/readonly-app-types
May 21, 2026
Merged

refactor(frontend): readonly on app-types cache/API shapes#1595
adamflagg merged 1 commit into
mainfrom
feature/readonly-app-types

Conversation

@adamflagg
Copy link
Copy Markdown
Owner

@adamflagg adamflagg commented 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/maps (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

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 Change Stack

§3 #8 second/final slice (graph.ts was #1592). 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 adopted 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: 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:
- buildCamperRows(campers): widened param to `readonly Camper[]` (it only
  filters/toSorted/maps — all return fresh arrays).
- RequestForm useState: copies `constraint.campers` into a mutable
  working copy `[...]`; the source stays immutable.

Verified: type-check green (both tsconfigs), csvExportHelpers suite 19/19,
full lefthook pre-push green. Closes out §3c #8; next live row is #9
(React Query queryOptions/skipToken).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c95a311e-f8bd-462e-9f1b-3082d064904a

📥 Commits

Reviewing files that changed from the base of the PR and between 1970b93 and 758c49c.

📒 Files selected for processing (4)
  • docs/reference/modernization-backlog.md
  • frontend/src/components/RequestForm.tsx
  • frontend/src/types/app-types.ts
  • frontend/src/utils/csvExportHelpers.ts

📝 Walkthrough

Walkthrough

Readonly immutability is added to six core frontend type interfaces (Camper, BunkRequest, Constraint, SolverRun, DragItem, BunkWithCampers), including deep nested expand and results fields. csvExportHelpers tightens its API contract to accept readonly arrays, and RequestForm adapts by cloning the readonly source. Backlog documentation is updated to mark #1592 complete.

Changes

Type Immutability and API Contracts

Layer / File(s) Summary
Core type immutability in app-types.ts
frontend/src/types/app-types.ts
File-level comment explains hand-written shapes use readonly (arrays and Readonly<Record>). Camper, BunkRequest, Constraint, SolverRun, DragItem, and BunkWithCampers interfaces add readonly to all fields, including nested expand and results structures, and convert mutable arrays/objects to readonly equivalents.
API contract tightening in csvExportHelpers
frontend/src/utils/csvExportHelpers.ts
buildCamperRows parameter updated from Camper[] to readonly Camper[].
RequestForm component adaptation
frontend/src/components/RequestForm.tsx
selectedCampers state initialization uses array spread to clone constraint.campers, with comment clarifying the local copy is mutable while the source is readonly.
Modernization backlog documentation
docs/reference/modernization-backlog.md
#1592 row updated to reflect both graph.ts and app-types.ts slices completed in this PR instead of app-types remaining open.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

  • adamflagg/kindred#1587: Updates buildCamperRows in csvExportHelpers to use non-mutating Array.prototype.toSorted(); paired with the readonly parameter contract change here.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/readonly-app-types

Comment @coderabbitai help to get the list of available commands and usage tips.

@adamflagg adamflagg merged commit 01db8f2 into main May 21, 2026
25 checks passed
@adamflagg adamflagg deleted the feature/readonly-app-types branch May 21, 2026 21:14
adamflagg added a commit that referenced this pull request May 21, 2026
#1598)

Closes #1596.

Finishes the app-types `readonly` contract opened by #1595 — same latent
readonly-leak class as the graph-type follow-ups #1593/#1594 (shipped in
#1597). Two consumers were intentionally left out of #1595 to keep that
a pure type-only change.

## Changes

1. **`AllCampersView.tsx`** — drop the now-redundant `as Camper[]` cast.
#1595 widened `buildCamperRows` to accept `readonly Camper[]`
(`csvExportHelpers.ts:73`), and `filteredCampers` is `MergedCamper[]`
(`MergedCamper extends Camper`), so it's directly assignable. The
standing cast only suppressed future compiler signal if the contract
changed.

2. **`mergeMultiSessionCampers.ts`** — mark `AdditionalSession`'s fields
and `MergedCamper.additionalSessions` `readonly`, matching the
now-`readonly` `Camper` that `MergedCamper` extends. The builder
constructs each `AdditionalSession` via `.map()` into fresh objects, so
the element type going `readonly` is a clean change.

Pure type-only hardening — **no runtime behavior change**, no active
mutation existed.

## Tests

Adds the first tests for `mergeMultiSessionCampers` (previously
untested):
- **Behavioral coverage** — multi-enrollment merge produces one entry
with `additionalSessions`; single-session camper untouched.
- **Readonly contract** — `@ts-expect-error` assertions enforced by `tsc
--noEmit`. Verified red→green: before the change the directives are
unused (TS2578); after, they type-check.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Tests**
* Added comprehensive test suite for multi-session camper merging
functionality, including validation of camper data consolidation and
immutability constraints.

* **Refactor**
* Enhanced type safety in CSV export and camper data handling to prevent
potential bugs and ensure data integrity during export operations.

<!-- review_stack_entry_start -->

[![Review Change
Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/adamflagg/kindred/pull/1598?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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant