From d8da1e2577c9c7d91802a1087c2c31bb96be2f64 Mon Sep 17 00:00:00 2001 From: Waleed Date: Sun, 21 Jun 2026 22:44:14 -0700 Subject: [PATCH] fix(state): align server/client state with best practices (query-key bugs, persist hygiene, useState) (#5166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(queries): close React Query key/fetch-arg drift cache collisions Several query hooks fetched with an identifier that was absent from their queryKey, so distinct fetch args shared one cache entry. Thread the missing args into the key factories and update all callsites/invalidations. - organization: useOrganization always fetched the ACTIVE org via getFullOrganization() while caching under detail(orgId). Pass orgId through to the better-auth call (query.organizationId); active-org behavior unchanged. - logs: logKeys.detail now keys on (workspaceId, logId) to prevent cross- workspace collision; updated useLogDetail, useLogByExecutionId, prefetchLogDetail, useCancelExecution optimistic path, and external callsites. - inbox: inboxKeys.taskList now includes cursor/limit (pagination args were sent but omitted from the key); keepPreviousData pagination UX preserved. - a2a: narrow create/update byWorkflows() invalidation to byWorkflow(ws, wf) since their responses reliably carry both ids; delete/publish stay broad. Not bugs (verified, left unchanged): - kb/connectors update/delete invalidate knowledgeKeys.detail(kbId), which is a prefix of connectorKeys.all(kbId) — connector list/detail are invalidated transitively by React Query prefix matching. Harness: add a key-fetch-arg-drift check to check-react-query-patterns.ts that flags a camelCase identifier the queryFn forwards into the fetch but is absent from the queryKey (excludes the requestJson contract arg, PascalCase/SCREAMING constants, and signal/pageParam machinery). Document the rule in sim-queries.md. tables.useTable annotated rq-lint-allow (tableId globally unique; workspaceId is only an authz scope). * fix(stores): whitelist durable fields in persist partialize chat/terminal/panel persist configs leaked actions and transient state into localStorage. Replace the chat full-state spread with an explicit durable whitelist, and add partialize to terminal and panel (which had none) so isResizing and _hasHydrated are no longer persisted. Panel keeps activeTab + panelWidth because the layout.tsx blocking script reads them from panel-state to set data-panel-active-tab before hydration (SSR tab-flash prevention). Harden sim-stores doctrine: persist MUST use an explicit partialize whitelist; never persist transient flags or _hasHydrated. * fix(state): model component useState as single source of truth - edit-knowledge-base-modal: reset fields on closed→open via prevOpenRef render idiom instead of mirroring props into state through useEffect (a prop change while open no longer clobbers in-progress edits) - use-verification: collapse contradictory isLoading/isVerified/isInvalidOtp booleans into a single status enum + errorMessage; consumer derives flags - contact-form / demo-request-modal: derive busy/success from the mutation object; delete duplicated submitSuccess local state - sim-hooks.md: add state-shape rule (no props-into-state, status enum, derive mutation state) * fix(verify): clear lingering message on complete OTP (restore parity) * docs(state): convert inline reset comment to TSDoc * docs(state): tighten harness rules for accuracy (queryFn forwards, partialize whitelist, mutation-flag caveat) * fix(verify): block auto-verify while a resend is in flight (restore parity) * fix(logs): key cancel optimistic detail by route workspaceId (not the log row) --- .claude/rules/sim-hooks.md | 16 +++ .claude/rules/sim-queries.md | 4 +- .claude/rules/sim-stores.md | 2 +- .../sim/app/(auth)/verify/use-verification.ts | 68 +++++---- apps/sim/app/(auth)/verify/verify-content.tsx | 17 ++- .../components/contact/contact-form.tsx | 5 +- .../demo-request/demo-request-modal.tsx | 5 +- .../resource-registry/resource-registry.tsx | 4 +- .../edit-knowledge-base-modal.tsx | 12 +- .../app/workspace/[workspaceId]/logs/logs.tsx | 4 +- apps/sim/hooks/queries/a2a/agents.ts | 28 ++-- apps/sim/hooks/queries/inbox.ts | 6 +- apps/sim/hooks/queries/logs.ts | 22 +-- apps/sim/hooks/queries/organization.ts | 16 ++- apps/sim/hooks/queries/tables.ts | 1 + apps/sim/stores/chat/store.ts | 14 +- apps/sim/stores/panel/store.ts | 11 ++ apps/sim/stores/terminal/store.ts | 13 ++ scripts/check-react-query-patterns.ts | 135 ++++++++++++++++++ 19 files changed, 307 insertions(+), 76 deletions(-) diff --git a/.claude/rules/sim-hooks.md b/.claude/rules/sim-hooks.md index c61119547c..6f600dfd94 100644 --- a/.claude/rules/sim-hooks.md +++ b/.claude/rules/sim-hooks.md @@ -48,3 +48,19 @@ export function useFeature({ id, onSelect }: UseFeatureProps) { 4. Wrap returned functions in useCallback 5. Server data goes through React Query (`hooks/queries/`), never `useState` + `fetch` 6. Keep only UI/orchestration state in these hooks + +## State shape + +Never mirror a prop into state with `useState(prop)` + a syncing `useEffect` — a prop change clobbers in-progress local edits. Use the prop directly, reset via a remount `key`, or — when you must seed local state from a prop only on a transition (e.g. a modal opening) — reset during render with the `prevX` ref idiom: + +```typescript +const prevOpenRef = useRef(open) +if (prevOpenRef.current !== open) { + prevOpenRef.current = open + if (open) setName(initialName) // closed → open only +} +``` + +Model mutually-exclusive flags as ONE `status` enum, not several contradictory booleans. `isLoading`/`isVerified`/`isInvalidOtp` describing one machine collapse to `status: 'idle' | 'verifying' | 'verified' | 'error'` (+ `errorMessage`); derive any boolean a consumer still needs (`status === 'error'`). + +Derive busy/success from the mutation object — never duplicate `mutation.isPending`/`mutation.isSuccess` into local `useState`. Read them directly (`mutation.isSuccess`) and reset with `mutation.reset()`. A distinct phase the mutation doesn't cover — e.g. a pre-submit captcha/Turnstile gate that runs before `mutate()` — is not a duplicate; keep that flag. diff --git a/.claude/rules/sim-queries.md b/.claude/rules/sim-queries.md index 1eb89ca5d1..25707c740a 100644 --- a/.claude/rules/sim-queries.md +++ b/.claude/rules/sim-queries.md @@ -25,6 +25,8 @@ export const entityKeys = { Never use inline query keys — always use the factory. +**Every identifier the `queryFn` forwards into the fetch MUST appear in the `queryKey`.** (Query-machinery identifiers — `signal`, `pageParam` — are exempt; they aren't fetch-scoping args.) If the fetch is scoped by `workspaceId`, `cursor`, `limit`, an org id, etc., those values must be part of the key — otherwise distinct fetch args share one cache entry (a cross-tenant / per-param cache collision). The lone exception is a globally-unique id used as the key while a second fetch arg is only an authz scope that cannot collide; annotate those with `// rq-lint-allow: `. Enforced by the `key-fetch-arg-drift` check in `scripts/check-react-query-patterns.ts`. + ## File Structure ```typescript @@ -142,4 +144,4 @@ const handler = useCallback(() => { ## Enforcement -`scripts/check-react-query-patterns.ts` (`bun run check:react-query`, run in CI) statically enforces these conventions: every `useQuery`/`useInfiniteQuery`/`useSuspenseQuery` declares an explicit `staleTime`, inline `queryFn`s destructure `signal`, `queryKey`s reference a colocated factory rather than an inline literal, and every `*Keys` factory in `hooks/queries/**` exposes an `all` root key. `hooks/queries/**` is a zero-tolerance zone; the rest of `apps/sim/**` is ratcheted against `scripts/check-react-query-patterns.baseline.json`. For a genuine exception, put `// rq-lint-allow: ` on the line directly above the flagged construct. +`scripts/check-react-query-patterns.ts` (`bun run check:react-query`, run in CI) statically enforces these conventions: every `useQuery`/`useInfiniteQuery`/`useSuspenseQuery` declares an explicit `staleTime`, inline `queryFn`s destructure `signal`, `queryKey`s reference a colocated factory rather than an inline literal, every `*Keys` factory in `hooks/queries/**` exposes an `all` root key, and every identifier the `queryFn` forwards into the fetch also appears in the `queryKey` (`key-fetch-arg-drift`). `hooks/queries/**` is a zero-tolerance zone; the rest of `apps/sim/**` is ratcheted against `scripts/check-react-query-patterns.baseline.json`. For a genuine exception, put `// rq-lint-allow: ` on the line directly above the flagged construct. diff --git a/.claude/rules/sim-stores.md b/.claude/rules/sim-stores.md index 333ff9fd91..273c394fbc 100644 --- a/.claude/rules/sim-stores.md +++ b/.claude/rules/sim-stores.md @@ -57,7 +57,7 @@ export const useFeatureStore = create()( 1. Use `devtools` middleware (named stores) 2. Use `persist` only when data should survive reload -3. `partialize` to persist only necessary state +3. `persist` MUST use `partialize` with an explicit whitelist of the durable fields. Exclude transient flags (`isResizing`, drag/hover state) and `_hasHydrated` from the whitelist, and never spread the whole state (`{ ...state }`) — it leaks actions and transient state into storage 4. `_hasHydrated` pattern for persisted stores needing hydration tracking 5. Immutable updates only 6. `set((state) => ...)` when depending on previous state diff --git a/apps/sim/app/(auth)/verify/use-verification.ts b/apps/sim/app/(auth)/verify/use-verification.ts index 7e809685ae..74c07ceae3 100644 --- a/apps/sim/app/(auth)/verify/use-verification.ts +++ b/apps/sim/app/(auth)/verify/use-verification.ts @@ -9,6 +9,15 @@ import { validateCallbackUrl } from '@/lib/core/security/input-validation' const logger = createLogger('useVerification') +/** + * Mutually-exclusive phases of the email-OTP verification machine. + * - `idle`: awaiting input + * - `verifying`: a verify request is in flight + * - `verified`: code accepted, redirecting + * - `error`: last verify attempt failed (paired with `errorMessage`) + */ +type VerificationStatus = 'idle' | 'verifying' | 'verified' | 'error' + interface UseVerificationParams { hasEmailService: boolean isProduction: boolean @@ -18,9 +27,8 @@ interface UseVerificationParams { interface UseVerificationReturn { otp: string email: string - isLoading: boolean - isVerified: boolean - isInvalidOtp: boolean + status: VerificationStatus + isResending: boolean errorMessage: string isOtpComplete: boolean hasEmailService: boolean @@ -41,10 +49,9 @@ export function useVerification({ const { refetch: refetchSession } = useSession() const [otp, setOtp] = useState('') const [email, setEmail] = useState('') - const [isLoading, setIsLoading] = useState(false) - const [isVerified, setIsVerified] = useState(false) + const [status, setStatus] = useState('idle') + const [isResending, setIsResending] = useState(false) const [isSendingInitialOtp, setIsSendingInitialOtp] = useState(false) - const [isInvalidOtp, setIsInvalidOtp] = useState(false) const [errorMessage, setErrorMessage] = useState('') const [redirectUrl, setRedirectUrl] = useState(null) const [isInviteFlow, setIsInviteFlow] = useState(false) @@ -96,8 +103,7 @@ export function useVerification({ async function verifyCode() { if (!isOtpComplete || !email) return - setIsLoading(true) - setIsInvalidOtp(false) + setStatus('verifying') setErrorMessage('') try { @@ -108,7 +114,7 @@ export function useVerification({ }) if (response && !response.error) { - setIsVerified(true) + setStatus('verified') try { await refetchSession() @@ -135,12 +141,9 @@ export function useVerification({ } else { logger.info('Setting invalid OTP state - API error response') const message = 'Invalid verification code. Please check and try again.' - setIsInvalidOtp(true) + setStatus('error') setErrorMessage(message) - logger.info('Error state after API error:', { - isInvalidOtp: true, - errorMessage: message, - }) + logger.info('Error state after API error:', { errorMessage: message }) setOtp('') } } catch (error: any) { @@ -155,23 +158,18 @@ export function useVerification({ message = 'Too many failed attempts. Please request a new code.' } - setIsInvalidOtp(true) + setStatus('error') setErrorMessage(message) - logger.info('Error state after caught error:', { - isInvalidOtp: true, - errorMessage: message, - }) + logger.info('Error state after caught error:', { errorMessage: message }) setOtp('') - } finally { - setIsLoading(false) } } function resendCode() { if (!email || !hasEmailService || !isEmailVerificationEnabled) return - setIsLoading(true) + setIsResending(true) setErrorMessage('') const normalizedEmail = normalizeEmail(email) @@ -185,32 +183,43 @@ export function useVerification({ setErrorMessage('Failed to resend verification code. Please try again later.') }) .finally(() => { - setIsLoading(false) + setIsResending(false) }) } + /** + * On a complete (6-char) code, clear any lingering message — including a + * resend failure (which sets `errorMessage` while `status` stays `idle`) — and + * exit the error state, matching the prior unconditional reset on a full OTP. + */ function handleOtpChange(value: string) { if (value.length === 6) { - setIsInvalidOtp(false) + if (status === 'error') setStatus('idle') setErrorMessage('') } setOtp(value) } useEffect(() => { - if (otp.length === 6 && email && !isLoading && !isVerified) { + if ( + otp.length === 6 && + email && + status !== 'verifying' && + status !== 'verified' && + !isResending + ) { const timeoutId = setTimeout(() => { verifyCode() }, 300) return () => clearTimeout(timeoutId) } - }, [otp, email, isLoading, isVerified]) + }, [otp, email, status, isResending]) useEffect(() => { if (typeof window !== 'undefined') { if (!isEmailVerificationEnabled) { - setIsVerified(true) + setStatus('verified') const handleRedirect = async () => { try { @@ -234,9 +243,8 @@ export function useVerification({ return { otp, email, - isLoading, - isVerified, - isInvalidOtp, + status, + isResending, errorMessage, isOtpComplete, hasEmailService, diff --git a/apps/sim/app/(auth)/verify/verify-content.tsx b/apps/sim/app/(auth)/verify/verify-content.tsx index b76e0f5faf..284af360a1 100644 --- a/apps/sim/app/(auth)/verify/verify-content.tsx +++ b/apps/sim/app/(auth)/verify/verify-content.tsx @@ -25,9 +25,8 @@ function VerificationForm({ const { otp, email, - isLoading, - isVerified, - isInvalidOtp, + status, + isResending, errorMessage, isOtpComplete, verifyCode, @@ -35,6 +34,10 @@ function VerificationForm({ handleOtpChange, } = useVerification({ hasEmailService, isProduction, isEmailVerificationEnabled }) + const isVerified = status === 'verified' + const isInvalidOtp = status === 'error' + const isBusy = status === 'verifying' || isResending + const [countdown, setCountdown] = useState(0) const [isResendDisabled, setIsResendDisabled] = useState(false) @@ -88,7 +91,7 @@ function VerificationForm({ maxLength={6} value={otp} onChange={handleOtpChange} - disabled={isLoading} + disabled={isBusy} className={cn('gap-2', isInvalidOtp && 'otp-error')} > @@ -112,10 +115,10 @@ function VerificationForm({ diff --git a/apps/sim/app/(landing)/components/contact/contact-form.tsx b/apps/sim/app/(landing)/components/contact/contact-form.tsx index 9474c9b9ab..b866b9e42c 100644 --- a/apps/sim/app/(landing)/components/contact/contact-form.tsx +++ b/apps/sim/app/(landing)/components/contact/contact-form.tsx @@ -69,7 +69,6 @@ export function ContactForm() { captureClientEvent('landing_contact_submitted', { topic: variables.topic }) setForm(INITIAL_FORM_STATE) setErrors({}) - setSubmitSuccess(true) }, onError: () => { turnstileRef.current?.reset() @@ -78,7 +77,6 @@ export function ContactForm() { const [form, setForm] = useState(INITIAL_FORM_STATE) const [errors, setErrors] = useState({}) - const [submitSuccess, setSubmitSuccess] = useState(false) const [isSubmitting, setIsSubmitting] = useState(false) const [website, setWebsite] = useState('') const [widgetReady, setWidgetReady] = useState(false) @@ -141,6 +139,7 @@ export function ContactForm() { } const isBusy = contactMutation.isPending || isSubmitting + const submitSuccess = contactMutation.isSuccess const submitError = contactMutation.isError ? toError(contactMutation.error).message || 'Failed to send message. Please try again.' @@ -161,7 +160,7 @@ export function ContactForm() {