Skip to content

refactor: apply nuclear-review findings (boundary validation, cart types, dedup) - #198

Merged
arzafran merged 5 commits into
mainfrom
refactor/nuclear-review-findings
Jun 8, 2026
Merged

refactor: apply nuclear-review findings (boundary validation, cart types, dedup)#198
arzafran merged 5 commits into
mainfrom
refactor/nuclear-review-findings

Conversation

@arzafran

@arzafran arzafran commented Jun 8, 2026

Copy link
Copy Markdown
Member

What this does

Applies the findings from a whole-codebase maintainability review (/nuclear-review). It's all behaviour-preserving cleanup — no features change — but it removes a class of latent bugs: malformed responses from HubSpot, Mailchimp, and Shopify now fail with a clear error at the network boundary instead of crashing later as a confusing property-access error, and the Shopify cart stops making an extra API call on every quantity change.

No user-facing behaviour changes. The success paths (form submits, cart add/remove/update, product fetch) work exactly as before.

Summary

Boundary validation (refactor(integrations))

  • New parseApiResponse(schema, json, context) helper in lib/utils/validation.ts; HubSpot, Mailchimp, and the Shopify GraphQL envelope are now zod-validated at the fetch boundary instead of (await res.json()) as T.
  • Mailchimp: typed MailchimpErrorCode union replaces error-string sniffing; tag/note writes are explicitly best-effort (no longer flip the result to failure); upsertMember dedups the two subscribe paths.
  • HubSpot: removed the dead hubspotType field and its orphaned typeSetter.
  • Turnstile: one dev-mode bypass path instead of two (production validation unchanged).

Shopify cart (refactor(shopify) + perf(shopify))

  • Named the post-reshape line item (CartLineItem), derived CartLine from it, made Cart.id required — removing 11 as casts the missing types forced, and guarding addCartItem against an undefined variant.
  • removeItem/updateItemQuantity take the lineId the client already holds, dropping the redundant getCart round-trip per mutation (a fallback is kept for optimistic lines without a confirmed id).
  • Synthetic "All" collection gets a stable updatedAt so it doesn't bust caches.

Scripts & utils (refactor(scripts) + refactor(utils))

  • ast-transforms: one shared in-memory ts-morph Project per applyOpsToText instead of up to six.
  • Removed a duplicate pascalCase, adopted the existing cancelGuard for clack prompts (dropping manual casts), and renamed the two divergent updatePackageJson functions.
  • Dropped the animation.ts re-export barrel so each util has one canonical import path.

Reviewed but intentionally not changed

  • Env-var allowlist vs blocklist (setup-project vs prepare-handoff): left separate. They diverge on purpose — setup preserves unknown custom vars (blocklist), handoff strips them (allowlist). Merging would break one guarantee.

Follow-up (out of scope, not a regression)

  • HubSpot forms feed the raw HubSpot fieldType into <input type=...>, so a phone field renders <input type="phone"> (invalid → falls back to text). Pre-existing; fixing it is a HubSpot-type → HTML-input-type mapping decision worth its own change.

Test Plan

  • bun run check green — biome (0 errors), tsgo --noEmit, 414 tests pass
  • lefthook pre-commit (typecheck + biome) passed on every commit
  • Manual: subscribe via a HubSpot/Mailchimp form; add/remove/change quantity in the Shopify cart

arzafran added 5 commits June 8, 2026 17:01
…oundaries

Add a shared parseApiResponse helper and use it to validate HubSpot,
Mailchimp, and Shopify responses at the fetch boundary instead of casting
(await res.json()) as T. Malformed responses now throw a descriptive error
naming the bad paths rather than crashing later as a property access.

- Mailchimp: replace error-string sniffing with a typed MailchimpErrorCode
  union; tag/note writes are now explicitly best-effort and no longer flip
  the result to failure; extract upsertMember to dedup the two subscribe paths.
- HubSpot: drop the unused hubspotType field and its orphaned typeSetter.
- Turnstile: collapse the duplicate dev-mode bypass so validateFormWithTurnstile
  delegates to validateTurnstile (production path unchanged).
Name the post-reshape line item (CartLineItem), derive CartLine from it, and
make Cart.id required so the reshape layer is typed end-to-end. Removes the
casts those missing types forced (cart-operations, reshape, actions, modal)
and guards the addCartItem reducer against an undefined variant.

removeItem/updateItemQuantity now take the lineId the client already holds,
skipping the extra getCart round-trip per mutation (a getCart fallback is kept
for optimistic lines whose server id is not yet confirmed). collections: give
the synthetic 'All' collection a stable updatedAt so it does not bust caches.
- ast-transforms: create a single in-memory Project per applyOpsToText and
  thread it through every handler instead of instantiating up to six.
- generate-manifest: reuse toPascalCase from generate-shared, drop the local
  duplicate.
- setup-project / prepare-handoff: adopt the existing cancelGuard for clack
  prompts (removing manual isCancel checks and casts); rename the two
  divergent updatePackageJson functions to describe what each does.
animation.ts re-exported clamp/lerp/mapRange/modulo/truncate from sibling util
modules, creating two import paths for the same functions. Keep only the
animation-owned exports and repoint the one importer (marquee) to @/utils/math.
Reflects the dropped animation.ts re-exports and the new parseApiResponse
helper. Fixes the manifest:check CI step.
@arzafran
arzafran merged commit 2d95327 into main Jun 8, 2026
8 checks passed
arzafran added a commit that referenced this pull request Jun 8, 2026
- CHANGELOG [Unreleased]: log the boundary validation + cart-type work
  (#198), the effort:low fixes (#205), the WebGL cleanup (#199/#206),
  lenis-prevent (#207), the Sanity 'use cache' fix (#208), and react-scan
  as an Orchestra toggle (#209).
- components/README: drop the deleted dropdown/ row (now Base UI Select).
- lib/dev/README: add the 🧊 webgl and 🔬 react-scan toggles to the
  Orchestra table; note react-scan is opt-in and how it relates to Stats.
- sanity/README: show the 'use cache' fetch pattern required under
  cacheComponents (bare sanityFetch throws cacheTag()).
arzafran added a commit that referenced this pull request Jun 9, 2026
- AGENTS.md: document the 'use cache' rule for cacheTag()-calling fetches
  (the #208 crash) and parseApiResponse boundary response-validation (#198)
- shopify README: updateItemQuantity/removeItem now take client-held lineId;
  note GraphQL envelope validation
- webgl README: flowmap/fluid sims are opt-in (#199, #206)
- biome-custom-rules: flag the cited dropdown example as since-removed (#202)
@arzafran
arzafran deleted the refactor/nuclear-review-findings branch June 22, 2026 14:18
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