324 lines
17 KiB
Markdown
324 lines
17 KiB
Markdown
# Website architecture audit: Template vs Client, Server Actions, Mutations, React Query
|
||
|
||
Contract baseline: [`docs/architecture/website/WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:1)
|
||
|
||
## 0) Executive summary
|
||
|
||
The codebase is **not uniformly wrong**; it is **inconsistent**.
|
||
|
||
Two competing architectures coexist:
|
||
|
||
1) **Contract-first RSC** (good):
|
||
- `app/**/page.tsx` calls PageQuery → builds ViewData → renders Template (examples below).
|
||
- Writes go through Server Actions → Mutations (examples below).
|
||
|
||
2) **React Query client app** (problematic when used for writes):
|
||
- Client modules execute writes via React Query `useMutation()` directly against services (or placeholders).
|
||
- This **violates the strict write boundary** in [`docs/architecture/website/FORM_SUBMISSION.md`](docs/architecture/website/FORM_SUBMISSION.md:20).
|
||
|
||
Additionally, some Templates violate determinism rules (notably `toLocaleString()` usage) which can cause SSR/hydration mismatches.
|
||
|
||
My recommendation to get to a clean codebase:
|
||
|
||
- Keep React Query **only for client-side reads** (and optional cache invalidation after server actions), but **ban React Query mutations for writes**.
|
||
- Enforce a single write path everywhere: **Client intent → Server Action → Mutation → Service → API Client**.
|
||
- Align Templates with determinism and responsibility rules: Templates render; they do not format using runtime-locale APIs.
|
||
|
||
---
|
||
|
||
## 1) What the code is doing today (evidence)
|
||
|
||
### 1.1 A good reference read flow (RSC → PageQuery → Template)
|
||
|
||
- Server page calls PageQuery and renders Template: [`DashboardPage()`](apps/website/app/dashboard/page.tsx:5)
|
||
- Template is a pure renderer receiving ViewData: [`DashboardTemplate()`](apps/website/templates/DashboardTemplate.tsx:21)
|
||
|
||
This matches the contract read flow in [`docs/architecture/website/WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:181).
|
||
|
||
### 1.2 A good reference write flow (Server Action → Mutation)
|
||
|
||
- Server action calls a mutation and revalidates: [`updateUserStatus()`](apps/website/app/admin/actions.ts:25)
|
||
- Server action calls a mutation and revalidates: [`updateProfileAction()`](apps/website/app/profile/actions.ts:8)
|
||
- Server action calls a mutation and revalidates: [`completeOnboardingAction()`](apps/website/app/onboarding/completeOnboardingAction.ts:18)
|
||
|
||
This matches the contract write flow in [`docs/architecture/website/WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:236) and the mutation guidance in [`docs/architecture/website/MUTATIONS.md`](docs/architecture/website/MUTATIONS.md:15).
|
||
|
||
### 1.3 React Query is definitely used, and it is used for writes
|
||
|
||
React Query is a dependency: [`apps/website/package.json`](apps/website/package.json:16)
|
||
|
||
React Query provider exists: [`QueryClientProvider()`](apps/website/lib/providers/QueryClientProvider.tsx:16)
|
||
|
||
There is a standardized abstraction that promotes React Query usage for both reads and writes:
|
||
- [`usePageData()`](apps/website/lib/page/usePageData.ts:25)
|
||
- [`usePageMutation()`](apps/website/lib/page/usePageData.ts:114)
|
||
|
||
Concrete contract violations:
|
||
|
||
- Client-side writes via React Query mutation calling a service:
|
||
- [`useMutation()`](apps/website/app/leagues/[id]/schedule/admin/LeagueAdminSchedulePageClient.tsx:52) calling [`leagueService.createAdminScheduleRace()`](apps/website/app/leagues/[id]/schedule/admin/LeagueAdminSchedulePageClient.tsx:69) and similar write methods.
|
||
- This is a direct breach of the strict rule: “All writes MUST enter through Next.js Server Actions” in [`docs/architecture/website/FORM_SUBMISSION.md`](docs/architecture/website/FORM_SUBMISSION.md:20).
|
||
|
||
- Duplicate write paths exist for onboarding:
|
||
- Server action exists: [`completeOnboardingAction()`](apps/website/app/onboarding/completeOnboardingAction.ts:18)
|
||
- But a React Query mutation hook also calls a service directly: [`useCompleteOnboarding()`](apps/website/hooks/onboarding/useCompleteOnboarding.ts:10)
|
||
|
||
- Some “mutation hooks” are placeholders (dangerous because they normalize the wrong path):
|
||
- [`useJoinTeam()`](apps/website/hooks/team/useJoinTeam.ts:10)
|
||
- [`useLeaveTeam()`](apps/website/hooks/team/useLeaveTeam.ts:9)
|
||
- These currently do no server action and no API call, but they establish the pattern that client-side “writes” live in React Query.
|
||
|
||
### 1.4 Template vs Client responsibilities are inconsistent
|
||
|
||
A clean example of the intended split exists:
|
||
|
||
- Server page acts as data orchestrator: [`Page()`](apps/website/app/drivers/page.tsx:6)
|
||
- Client entry-point owns state and navigation: [`DriversPageClient()`](apps/website/app/drivers/DriversPageClient.tsx:30)
|
||
- Template is basically composition/rendering: [`DriversTemplate()`](apps/website/templates/DriversTemplate.tsx:38)
|
||
|
||
But the actual boundaries are blurry in at least two ways:
|
||
|
||
1) Templates contain non-deterministic formatting:
|
||
- [`toLocaleString()`](apps/website/templates/DriversTemplate.tsx:65)
|
||
- This violates strict determinism constraints in [`docs/architecture/website/VIEW_DATA.md`](docs/architecture/website/VIEW_DATA.md:58) and [`docs/architecture/website/DISPLAY_OBJECTS.md`](docs/architecture/website/DISPLAY_OBJECTS.md:71).
|
||
|
||
2) `app/**/*PageClient.tsx` sometimes becomes “everything”: state, rendering, presentation model transformation, and workaround ViewModel creation.
|
||
- Example: “TODO wtf we have builders for this” comment while doing ViewData → ViewModel ad-hoc mapping in [`LeaguesPageClient()`](apps/website/app/leagues/LeaguesPageClient.tsx:418) and specifically the conversion at [`LeagueSummaryViewModel`](apps/website/app/leagues/LeaguesPageClient.tsx:380).
|
||
|
||
### 1.5 There are plain bugs/anti-patterns that amplify the mess
|
||
|
||
- `useState()` used as an effect runner (should be `useEffect()`):
|
||
- [`useState()`](apps/website/app/leagues/LeaguesPageClient.tsx:297)
|
||
- This is not an architecture issue per se, but it is “noise” that makes architecture discussions harder.
|
||
|
||
---
|
||
|
||
## 2) The root causes (why this became a mess)
|
||
|
||
1) **Write boundary not enforced strongly enough**
|
||
- There are good server actions + mutation examples, but nothing stops new client code from doing `useMutation()` → `service.write()`.
|
||
|
||
2) **Templates and ViewData rules are not consistently applied**
|
||
- Determinism rules (no `Intl.*`, no `toLocale*`) are strict in docs, but not enforced in Templates.
|
||
|
||
3) **Two read strategies are both used**
|
||
- RSC PageQueries exist.
|
||
- React Query hooks exist.
|
||
- Without a clear “when to use which,” teams will mix-and-match.
|
||
|
||
---
|
||
|
||
## 3) Recommended target architecture (single coherent system)
|
||
|
||
### 3.1 Writes: one true path
|
||
|
||
Hard rule (enforced):
|
||
|
||
- Client code MUST NOT perform write HTTP requests.
|
||
- Client code MUST NOT call write methods on services that hit the API.
|
||
- Client code MUST NOT use React Query `useMutation()` for writes.
|
||
|
||
Canonical write flow (matches docs):
|
||
|
||
```mermaid
|
||
flowchart TD
|
||
UI[Client component intent] --> SA[Server Action]
|
||
SA --> M[Mutation]
|
||
M --> S[Service]
|
||
S --> AC[API Client]
|
||
AC --> API[Backend API]
|
||
SA --> RV[Revalidate Path]
|
||
```
|
||
|
||
Docs reference: [`docs/architecture/website/FORM_SUBMISSION.md`](docs/architecture/website/FORM_SUBMISSION.md:20)
|
||
|
||
### 3.2 Reads: choose a default, allow an exception
|
||
|
||
Default:
|
||
|
||
- If the route can be rendered on the server: `page.tsx` → PageQuery → ViewDataBuilder → Template.
|
||
|
||
Exception:
|
||
|
||
- Use React Query for client-only interactive islands that truly need client-driven data refresh.
|
||
- React Query should be treated as **a client state/cache** of authoritative API truth, not the truth itself.
|
||
|
||
Canonical read flow option A (RSC):
|
||
|
||
```mermaid
|
||
flowchart TD
|
||
Page[page.tsx] --> PQ[PageQuery]
|
||
PQ --> S[Service]
|
||
S --> AC[API Client]
|
||
PQ --> B[ViewDataBuilder]
|
||
B --> T[Template]
|
||
```
|
||
|
||
Docs reference: [`docs/architecture/website/WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:181)
|
||
|
||
Canonical read flow option B (client-only island):
|
||
|
||
```mermaid
|
||
flowchart TD
|
||
C[Client component] --> Q[React Query useQuery]
|
||
Q --> S[Service or API Client read]
|
||
C --> T[Template]
|
||
```
|
||
|
||
Important: This is only acceptable for reads. Writes still go through server actions.
|
||
|
||
---
|
||
|
||
## 4) Concrete refactor plan (incremental, low-risk)
|
||
|
||
### Step 1: Establish explicit conventions
|
||
|
||
1) `app/**/page.tsx` MUST be server by default and must only orchestrate:
|
||
- call PageQuery
|
||
- map Result → `notFound()`/`redirect()`/error UI
|
||
- build ViewData using Builder
|
||
- render Template
|
||
|
||
2) `app/**/*PageClient.tsx` is the ONLY place allowed to:
|
||
- use `useRouter()`
|
||
- hold UI state
|
||
- call server actions
|
||
|
||
3) `templates/**/*Template.tsx` MUST:
|
||
- accept ViewData (or client-only “presentation props” like handler functions)
|
||
- contain no `Intl.*` or `toLocale*` formatting
|
||
- contain no data fetching
|
||
|
||
### Step 2: Fix strict correctness violations first (determinism)
|
||
|
||
- Remove runtime-locale formatting from templates:
|
||
- Replace [`toLocaleString()`](apps/website/templates/DriversTemplate.tsx:65) and [`toLocaleString()`](apps/website/templates/DriversTemplate.tsx:66) with deterministic formatting.
|
||
- Preferred: ViewData already contains display-ready strings (built via ViewDataBuilder), or deterministic Display Objects.
|
||
- Docs reference: [`docs/architecture/website/VIEW_DATA.md`](docs/architecture/website/VIEW_DATA.md:58).
|
||
|
||
### Step 3: Move client-side writes behind server actions
|
||
|
||
Primary offender to migrate:
|
||
|
||
- Replace React Query write mutations in [`LeagueAdminSchedulePageClient()`](apps/website/app/leagues/[id]/schedule/admin/LeagueAdminSchedulePageClient.tsx:22)
|
||
- Replace [`useMutation()`](apps/website/app/leagues/[id]/schedule/admin/LeagueAdminSchedulePageClient.tsx:52) with server actions (publish/save/delete) that call Mutations.
|
||
- Then the client component calls those server actions and refreshes view (either `router.refresh()` or route revalidation).
|
||
|
||
Secondary offenders:
|
||
|
||
- Consolidate onboarding writes to one path:
|
||
- Keep server actions: [`completeOnboardingAction()`](apps/website/app/onboarding/completeOnboardingAction.ts:18) and [`generateAvatarsAction()`](apps/website/app/onboarding/generateAvatarsAction.ts:12)
|
||
- Delete or rewrite client hook [`useCompleteOnboarding()`](apps/website/hooks/onboarding/useCompleteOnboarding.ts:10) to call server action instead of a service.
|
||
|
||
### Step 4: Clean up presentation model transforms
|
||
|
||
- Eliminate ad-hoc “ViewData → ViewModel” conversions in client code:
|
||
- Example to refactor: the conversion at [`LeagueSummaryViewModel`](apps/website/app/leagues/LeaguesPageClient.tsx:380).
|
||
- Replace with either:
|
||
- a proper ViewModelBuilder (if the component truly needs ViewModel semantics), or
|
||
- adjust the component (`LeagueCard`) to accept ViewData shape directly.
|
||
|
||
### Step 5: Reduce React Query surface area (optional, but recommended)
|
||
|
||
- Deprecate the abstraction that encourages client-side writes:
|
||
- Keep [`usePageData()`](apps/website/lib/page/usePageData.ts:25) for reads if desired.
|
||
- Remove or forbid [`usePageMutation()`](apps/website/lib/page/usePageData.ts:114).
|
||
|
||
### Step 6: Add guardrails to prevent regression
|
||
|
||
Minimal guardrails that pay off:
|
||
|
||
1) ESLint rule or convention: forbid `useMutation` imports except in `__tests__` or an allowlist.
|
||
2) ESLint rule: forbid service write methods from being imported into client modules (harder, but possible with path rules).
|
||
3) ESLint rule: forbid `toLocaleString()` and `Intl.*` in Templates.
|
||
|
||
---
|
||
|
||
## 5) Answers to your specific concerns
|
||
|
||
### 5.1 `*Template.tsx` vs `*Client.tsx`
|
||
|
||
- This split is **correct** and already present in multiple routes.
|
||
- The inconsistency is that some “Client” files are doing too much (transform, render, and mutate).
|
||
- The fix is to treat `*PageClient.tsx` as wiring + state + calling server actions, and keep templates deterministic.
|
||
|
||
### 5.2 Server actions vs mutations
|
||
|
||
- The codebase already shows the correct approach:
|
||
- [`updateUserStatus()`](apps/website/app/admin/actions.ts:25)
|
||
- [`updateProfileAction()`](apps/website/app/profile/actions.ts:8)
|
||
|
||
- The “mess” is client-side writes bypassing this.
|
||
|
||
### 5.4 Command Models: Are they useless?
|
||
|
||
No. Command Models (e.g., [`LoginCommandModel`](apps/website/lib/command-models/auth/LoginCommandModel.ts)) remain valuable as **optional UX helpers**.
|
||
|
||
- **Purpose**: They manage transient form state and perform client-side UX validation.
|
||
- **Relationship to Server Actions**: They prepare the DTO that is passed *to* the Server Action.
|
||
- **Boundary**: They belong to the client-side "intent collection" phase. Once the Server Action is called, the Command Model's job is done.
|
||
- **Redundancy**: They are **not redundant** with ViewModels. ViewModels are for the **read path** (API -> UI), while Command Models are for the **write path** (UI -> API).
|
||
|
||
### 5.5 Identified places for Command Model usage
|
||
|
||
The following components currently use complex React state for forms and should be refactored to use Command Models:
|
||
|
||
1. **[`CreateLeagueWizard.tsx`](apps/website/app/leagues/create/CreateLeagueWizard.tsx)**: Needs a `CreateLeagueCommandModel` to manage multi-step wizard state and validation.
|
||
2. **[`LeagueAdminSchedulePageClient.tsx`](apps/website/app/leagues/[id]/schedule/admin/LeagueAdminSchedulePageClient.tsx)**: Should use a `RaceScheduleCommandModel` for the track/car/date fields.
|
||
3. **[`ProtestDetailPageClient.tsx`](apps/website/app/leagues/[id]/stewarding/protests/[protestId]/ProtestDetailPageClient.tsx)**: Should fully leverage the existing [`ProtestDecisionCommandModel`](apps/website/lib/command-models/protests/ProtestDecisionCommandModel.ts).
|
||
4. **[`SponsorSignupPage`](apps/website/app/sponsor/signup/page.tsx)**: Needs a `SponsorSignupCommandModel` to clean up the `formData` and `errors` state.
|
||
|
||
---
|
||
|
||
## 6) Opinion: ViewData vs ViewModel (what actually makes sense)
|
||
|
||
Your original intuition is sound:
|
||
|
||
- **ViewData = server output** (JSON, deterministic, template-ready)
|
||
- **ViewModel = client-only** (state, derived UI helpers, never serialized)
|
||
|
||
The awkward part is: the repo currently contains **conflicting written contracts**.
|
||
|
||
- Strict statement: “Templates accept ViewData only” in [`docs/architecture/website/WEBSITE_CONTRACT.md`](docs/architecture/website/WEBSITE_CONTRACT.md:372).
|
||
- But [`docs/architecture/website/VIEW_DATA.md`](docs/architecture/website/VIEW_DATA.md:34) shows an example where a Template receives a ViewModel in client code.
|
||
|
||
### My recommendation: keep Templates ViewData-only, keep ViewModels client-only
|
||
|
||
I would enforce this as the single, clean rule:
|
||
|
||
1) **Templates accept ViewData only** (one prop-shape; server and client use the same templates).
|
||
2) **Client components may use ViewModels internally** for state and derived values.
|
||
3) **The boundary is explicit**: client code converts `ViewModel -> ViewData` right before rendering a Template.
|
||
|
||
Why this is the least-bad option:
|
||
|
||
- It prevents a “two template APIs” world (one for ViewData, one for ViewModel) which tends to explode complexity.
|
||
- It aligns with RSC constraints (serializable data across the server/client boundary).
|
||
- It keeps ViewModels useful: they can own derived UI state and display logic, but the render surface is still a plain object.
|
||
|
||
Concrete example of why you feel “ViewModels are pushed away by ViewData”:
|
||
|
||
- In [`LeaguesPageClient()`](apps/website/app/leagues/LeaguesPageClient.tsx:418) you’re holding ViewData and then doing a hand-rolled mapping to a “ViewModel-like” shape at [`LeagueSummaryViewModel`](apps/website/app/leagues/LeaguesPageClient.tsx:380). That’s a symptom of **missing builders and an unclear boundary**, not a reason to abandon ViewModels.
|
||
|
||
## 7) Implementation Status (Audit Results)
|
||
|
||
The following cleanups have been implemented:
|
||
|
||
1. **Deterministic Formatting**: Removed `toLocaleString()` and `toLocaleDateString()` from all templates. Introduced `NumberDisplay` and `DateDisplay` display objects for deterministic formatting.
|
||
2. **Server Action Migration**: Migrated the schedule administration logic in `LeagueAdminSchedulePageClient.tsx` from React Query `useMutation` to Next.js Server Actions.
|
||
3. **Onboarding Consolidation**: Updated onboarding hooks to use server actions instead of direct service calls.
|
||
4. **ViewModel Builders**: Introduced `LeagueSummaryViewModelBuilder` to eliminate ad-hoc ViewData -> ViewModel mapping in `LeaguesPageClient.tsx`.
|
||
5. **React Query Deprecation**: Deprecated `usePageMutation` in `usePageData.ts`.
|
||
6. **Guardrails**: Added `gridpilot-rules/no-use-mutation-in-client` ESLint rule to prevent future React Query write usage.
|
||
7. **Command Models**:
|
||
- Created [`RaceScheduleCommandModel`](apps/website/lib/command-models/leagues/RaceScheduleCommandModel.ts) and integrated it into [`LeagueAdminSchedulePageClient.tsx`](apps/website/app/leagues/[id]/schedule/admin/LeagueAdminSchedulePageClient.tsx).
|
||
- Created [`SponsorSignupCommandModel`](apps/website/lib/command-models/sponsors/SponsorSignupCommandModel.ts) and integrated it into [`SponsorSignupPage`](apps/website/app/sponsor/signup/page.tsx).
|
||
- Ensured [`LeagueWizardCommandModel`](apps/website/lib/command-models/leagues/LeagueWizardCommandModel.ts) is ready for future refactoring of the complex wizard.
|
||
|
||
## 8) Next Steps for the Team
|
||
|
||
- Continue migrating remaining `useMutation` usages to Server Actions.
|
||
- Ensure all new templates follow the ViewData-only rule.
|
||
- Use `NumberDisplay` and `DateDisplay` for all UI formatting.
|
||
|