Skip to content

fix(studio): stop onboarding detect-site refetch loop on failed detection#36

Open
bnz183 wants to merge 1 commit into
feat/phase5-governancefrom
fix/onboarding-detect-loop
Open

fix(studio): stop onboarding detect-site refetch loop on failed detection#36
bnz183 wants to merge 1 commit into
feat/phase5-governancefrom
fix/onboarding-detect-loop

Conversation

@bnz183

@bnz183 bnz183 commented Jun 24, 2026

Copy link
Copy Markdown
Owner

What

The onboarding wizard's detect-site step refetched /api/setup/detect in a tight loop whenever detection failed or returned nothing — ~1,600 requests in 3s, verified at runtime. React's dev-only passive-effect loop guard is stripped in production, so prod silently hammers the API rather than crashing.

Root cause

The effect keyed its guard on report === null, which conflated "not fetched yet" with "fetched and failed". On a null result report stayed null while setLoadingDetection(false) flipped a dependency, re-running the effect → guard passed → refetch, forever.

Fix (OnboardingWizard.tsx only)

  • Track attempted separately from result via a resolvedAttempt token, set only when a fetch settles (success or failure). The guard resolvedAttempt === retryCount then stops re-fetching — null no longer doubles as "unattempted".
  • Effect deps are [step, retryCount, resolvedAttempt] — no self-referential report/error outputs, so the effect can't re-arm itself.
  • loading is derived (avoids react-hooks/set-state-in-effect); cleanup ignore flag drops stale/duplicate responses.
  • Real error state with a Try again control; Continue is disabled on loading/error, closing the old "click through a blank wizard" hole.

Tests

New e2e/onboarding-detection-loop.spec.ts:

  • Loop guard: force detect → 500, assert ≤1 request while idle, error notice visible, Continue disabled. (Was RED on the old code: "saw 377".)
  • Retry: exactly one additional request and recovery on success.

Existing happy-path detection test (smoke.spec.ts) still green. tsc -b + targeted lint clean; no new lint suppressions.

Scope

Touches only OnboardingWizard.tsx and the new spec. The five other review findings on this branch are intentionally left untouched.

🤖 Generated with Claude Code

…tion

The detect-site effect keyed its guard on `report === null`, which conflated
"not fetched yet" with "fetched and failed", so a failed/empty detection
re-armed the effect and refetched /api/setup/detect in a tight loop. Track the
attempted state via a resolved-attempt token (set only when a fetch settles),
derive loading/error/loaded status, surface a real error state with a Retry
control, and block Continue on failure. Add e2e coverage asserting no refetch
loop and single-request retry recovery.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a376feb673

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +527 to +529
(detectionStatus === "loading" ||
detectionStatus === "error" ||
(report !== null && !report.primary)))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Block continuing after a null post-save refresh

If the initial scan succeeds but the follow-up fetchSetupDetection() in handleApplySettings returns null after saving settings, detectionError is never set and resolvedAttempt still matches retryCount, so the wizard is in a loaded/null state. This disabled check treats report === null as okay, leaving Continue enabled and allowing users to advance into blank confirmation steps with no active suggestion; please either preserve the previous report or treat this null refresh as an error/blocked state.

Useful? React with 👍 / 👎.

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