Files
gridpilot.gg/plans/website-architecture-audit.md
2026-01-16 01:32:55 +01:00

15 KiB
Raw Blame History

Website architecture audit: Template vs Client, Server Actions, Mutations, React Query

Contract baseline: docs/architecture/website/WEBSITE_CONTRACT.md

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):

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)

This matches the contract read flow in docs/architecture/website/WEBSITE_CONTRACT.md.

1.2 A good reference write flow (Server Action → Mutation)

This matches the contract write flow in docs/architecture/website/WEBSITE_CONTRACT.md and the mutation guidance in docs/architecture/website/MUTATIONS.md.

1.3 React Query is definitely used, and it is used for writes

React Query is a dependency: apps/website/package.json

React Query provider exists: QueryClientProvider()

There is a standardized abstraction that promotes React Query usage for both reads and writes:

Concrete contract violations:

1.4 Template vs Client responsibilities are inconsistent

A clean example of the intended split exists:

But the actual boundaries are blurry in at least two ways:

  1. Templates contain non-deterministic formatting:

  2. app/**/*PageClient.tsx sometimes becomes “everything”: state, rendering, presentation model transformation, and workaround ViewModel creation.

1.5 There are plain bugs/anti-patterns that amplify the mess

  • useState() used as an effect runner (should be useEffect()):
    • useState()
    • 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.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):

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

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):

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

Canonical read flow option B (client-only island):

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)

Step 3: Move client-side writes behind server actions

Primary offender to migrate:

  • Replace React Query write mutations in LeagueAdminSchedulePageClient()
    • Replace useMutation() 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:

Step 4: Clean up presentation model transforms

  • Eliminate ad-hoc “ViewData → ViewModel” conversions in client code:
    • Example to refactor: the conversion at LeagueSummaryViewModel.
    • Replace with either:
      • a proper ViewModelBuilder (if the component truly needs ViewModel semantics), or
      • adjust the component (LeagueCard) to accept ViewData shape directly.

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

5.3 React Query usage: is it even used?

Yes.

  • React Query exists in dependencies: apps/website/package.json
  • There are dozens of hooks using it (search results in workspace).

The problem is not React Query for reads. The problem is React Query being used as the write mechanism.


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.

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() youre holding ViewData and then doing a hand-rolled mapping to a “ViewModel-like” shape at LeagueSummaryViewModel. Thats 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 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.

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.