From a3eafecf6011b9a9d9febfa3d5d1cd6cf2a5dcf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Sun, 9 Aug 2026 18:40:49 +0200 Subject: [PATCH] fix(onboarding): don't trap users on the syncing step when a bank sync fails (#745) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by a user who spent over an hour and a half on the onboarding syncing step, on two devices, without ever getting into the app. ## What was happening When Enable Banking rate limits a connection, `SyncBankingConnectionJob` records the error and backs off, but leaves the connection **Active with `last_synced_at` still NULL** — the exact shape `syncStatus` treated as "still syncing". The step polled that endpoint forever, and there was no deadline or escape on the client. Confirmed in production for the reporter: a Trade Republic connection returned `429` on every scheduled sync for two days straight. They unblocked themselves by deleting the connection — `onboarded_at` was set four seconds later. Three other users are in the same state right now, one of them from today. ## What changed - **`syncStatus` no longer waits on a connection that already failed**, and reports it as `failed` separately from `pending`. - **The step says so instead of pretending it worked.** Advancing silently dropped the user on a "You are all set!" screen with an empty dashboard and no way to find out why. Now they get a short explanation and a Continue button. - **Client deadline raised to 5 minutes** — the previous 2 was under the worst case of a healthy first sync (3 attempts of a 120s job, 30s apart) — and the status request now has a timeout, so a hanging poll can't stall the step either. A failing status check keeps polling until the deadline rather than skipping the user ahead. - **`AccountMappingController` clears the stale error** when it reactivates a connection, so a genuine sync in flight isn't reported as failed. ## Testing - Feature tests for the rate-limited case, for a healthy connection syncing alongside a failed one, and for the failed flag; a `rateLimited()` factory state replaces the hand-rolled attributes. - Vitest coverage for the three client branches: failed, deadline reached, and the normal advance. - Browser QA against the real app, reproducing the production state: the honest screen appears instead of the spinner, Continue moves on, the user completes onboarding and reaches the app; and a genuinely pending sync still spins and then advances by itself once the connection syncs. No console errors. ## Demo https://github.com/user-attachments/assets/e54e029d-db7a-47b0-8b93-0da5569aa363 ## Not in this PR `settings/connections.tsx` has the same `active && !last_synced_at` = "syncing" heuristic, so a rate-limited connection shows a permanent green "Syncing" badge there and its error is never rendered (the error block is gated on `status === 'error'`). Same bug class, separate surface — worth a follow-up. --- app/Http/Controllers/OnboardingController.php | 25 +++++- .../OpenBanking/AccountMappingController.php | 2 + .../factories/BankingConnectionFactory.php | 14 ++++ lang/es.json | 2 + .../onboarding/step-syncing.test.tsx | 81 +++++++++++++++++++ .../js/components/onboarding/step-syncing.tsx | 67 ++++++++++++--- .../Onboarding/OnboardingSyncStatusTest.php | 40 +++++++++ 7 files changed, 217 insertions(+), 14 deletions(-) create mode 100644 resources/js/components/onboarding/step-syncing.test.tsx diff --git a/app/Http/Controllers/OnboardingController.php b/app/Http/Controllers/OnboardingController.php index 7bb3a62a..3689b520 100644 --- a/app/Http/Controllers/OnboardingController.php +++ b/app/Http/Controllers/OnboardingController.php @@ -5,6 +5,7 @@ namespace App\Http\Controllers; use App\Enums\BankingConnectionStatus; use App\Jobs\CategorizeOnboardingTransactionsJob; use App\Models\Bank; +use App\Models\BankingConnection; use App\Models\Category; use App\Models\Transaction; use Illuminate\Http\JsonResponse; @@ -75,15 +76,31 @@ class OnboardingController extends Controller ]); } + /** + * Report whether the onboarding sync step should keep waiting. + * + * A connection that already recorded an error will never set last_synced_at + * (rate limits keep the status Active while only storing the message), so it + * must not hold the user on the syncing step forever. It is reported as + * failed instead, so the step can say so rather than claim everything worked. + */ public function syncStatus(Request $request): JsonResponse { - $pending = $request->user() + $unsynced = $request->user() ->bankingConnections() - ->where('status', BankingConnectionStatus::Active) + ->whereIn('status', [BankingConnectionStatus::Active, BankingConnectionStatus::Error]) ->whereNull('last_synced_at') - ->exists(); + ->get(['status', 'error_message']); - return response()->json(['pending' => $pending]); + $pending = $unsynced->contains( + fn (BankingConnection $connection): bool => $connection->status === BankingConnectionStatus::Active + && $connection->error_message === null + ); + + return response()->json([ + 'pending' => $pending, + 'failed' => ! $pending && $unsynced->isNotEmpty(), + ]); } public function complete(Request $request): RedirectResponse diff --git a/app/Http/Controllers/OpenBanking/AccountMappingController.php b/app/Http/Controllers/OpenBanking/AccountMappingController.php index 558ee0b1..bf65449e 100644 --- a/app/Http/Controllers/OpenBanking/AccountMappingController.php +++ b/app/Http/Controllers/OpenBanking/AccountMappingController.php @@ -126,6 +126,8 @@ class AccountMappingController extends Controller $connection->update([ 'status' => BankingConnectionStatus::Active, 'pending_accounts_data' => null, + 'error_message' => null, + 'consecutive_sync_failures' => 0, ]); SyncBankingConnectionJob::dispatch($connection); diff --git a/database/factories/BankingConnectionFactory.php b/database/factories/BankingConnectionFactory.php index 71edb947..6b55de10 100644 --- a/database/factories/BankingConnectionFactory.php +++ b/database/factories/BankingConnectionFactory.php @@ -171,4 +171,18 @@ class BankingConnectionFactory extends Factory 'consecutive_sync_failures' => 1, ]); } + + /** + * A connection the provider rate limited: still Active, backing off, and + * carrying an error message instead of a sync. + */ + public function rateLimited(): static + { + return $this->state(fn (array $attributes) => [ + 'status' => BankingConnectionStatus::Active, + 'last_synced_at' => null, + 'rate_limited_until' => now()->addHour(), + 'error_message' => 'Rate limit exceeded. Please wait a few minutes and try again.', + ]); + } } diff --git a/lang/es.json b/lang/es.json index 1b149aa4..7fe492b8 100644 --- a/lang/es.json +++ b/lang/es.json @@ -1983,6 +1983,7 @@ "Your :provider connection has expired.": "Tu conexión con :provider ha caducado.", "Your :provider connection needs attention": "Tu conexión con :provider necesita atención", "Your Accounts": "Tus Cuentas", + "Your bank is taking longer than expected. We’ll keep trying in the background and your transactions will show up automatically.": "Tu banco está tardando más de lo previsto. Seguiremos intentándolo en segundo plano y tus transacciones aparecerán automáticamente.", "Your Categories Include:": "Tus Categorías Incluyen:", "Your Data is Truly Private": "Tus Datos Son Verdaderamente Privados", "Your Data, Your Privacy": "Tus Datos, Tu Privacidad", @@ -2178,6 +2179,7 @@ "No thanks": "No, gracias", "AI suggestions need more data": "Las sugerencias de IA necesitan más datos", "Once you have at least :count transactions, you can generate rule suggestions from Settings → Automation rules.": "Cuando tengas al menos :count transacciones, podrás generar sugerencias de reglas desde Ajustes → Reglas de automatización.", + "We couldn’t finish importing right now": "No hemos podido terminar la importación", "We couldn’t generate suggestions": "No pudimos generar sugerencias", "Something went wrong. You can try again or skip for now.": "Algo salió mal. Puedes intentarlo de nuevo u omitirlo por ahora.", "Try again": "Intentar de nuevo", diff --git a/resources/js/components/onboarding/step-syncing.test.tsx b/resources/js/components/onboarding/step-syncing.test.tsx new file mode 100644 index 00000000..a09c4bcf --- /dev/null +++ b/resources/js/components/onboarding/step-syncing.test.tsx @@ -0,0 +1,81 @@ +import { act, render, screen } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { StepSyncing } from './step-syncing'; + +const { get } = vi.hoisted(() => ({ get: vi.fn() })); + +vi.mock('axios', () => ({ + default: { get, isAxiosError: () => false }, +})); + +const reload = vi.fn(); + +vi.mock('@inertiajs/react', () => ({ + router: { reload: (...args: unknown[]) => reload(...args) }, +})); + +describe('StepSyncing', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + get.mockReset(); + reload.mockReset(); + }); + + it('stops spinning and offers to continue when a connection failed', async () => { + get.mockResolvedValue({ data: { pending: false, failed: true } }); + + render(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + + expect( + screen.getByText('We couldn’t finish importing right now'), + ).toBeInTheDocument(); + expect(reload).not.toHaveBeenCalled(); + }); + + it('gives up on a sync that never resolves instead of polling forever', async () => { + get.mockResolvedValue({ data: { pending: true, failed: false } }); + + render(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect( + screen.getByText('This will only take a moment.'), + ).toBeInTheDocument(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(5 * 60_000 + 3_000); + }); + + expect( + screen.getByText('We couldn’t finish importing right now'), + ).toBeInTheDocument(); + expect( + screen.queryByText('This will only take a moment.'), + ).not.toBeInTheDocument(); + }); + + it('advances on its own once every connection has synced', async () => { + get.mockResolvedValue({ data: { pending: false, failed: false } }); + + render(); + + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + + expect(reload).toHaveBeenCalled(); + expect( + screen.queryByText('We couldn’t finish importing right now'), + ).not.toBeInTheDocument(); + }); +}); diff --git a/resources/js/components/onboarding/step-syncing.tsx b/resources/js/components/onboarding/step-syncing.tsx index 9ab788cd..9c6c9048 100644 --- a/resources/js/components/onboarding/step-syncing.tsx +++ b/resources/js/components/onboarding/step-syncing.tsx @@ -1,10 +1,19 @@ +import { StepButton } from '@/components/onboarding/step-button'; +import { StepHeader } from '@/components/onboarding/step-header'; import { syncStatus } from '@/routes/onboarding'; import { __ } from '@/utils/i18n'; import { router } from '@inertiajs/react'; import axios from 'axios'; -import { Loader2 } from 'lucide-react'; +import { CloudOff, Loader2 } from 'lucide-react'; import { useCallback, useEffect, useRef, useState } from 'react'; +// Client-side give-up: the sync keeps running on the queue, but this guarantees +// the spinner resolves even if the worker dies. Beyond the worst case a healthy +// first sync can take (3 attempts of a 120s job, 30s apart). +const MAX_POLL_MS = 5 * 60_000; + +const POLL_INTERVAL_MS = 3_000; + const MESSAGES = [ 'Importing your balances...', 'Fetching your transactions...', @@ -24,6 +33,7 @@ interface StepSyncingProps { export function StepSyncing({ onComplete }: StepSyncingProps) { const [messageIndex, setMessageIndex] = useState(0); const [isPending, setIsPending] = useState(null); + const [hasStalled, setHasStalled] = useState(false); const onCompleteRef = useRef(onComplete); onCompleteRef.current = onComplete; @@ -40,28 +50,49 @@ export function StepSyncing({ onComplete }: StepSyncingProps) { useEffect(() => { let cancelled = false; let pollTimer: ReturnType; + const deadline = Date.now() + MAX_POLL_MS; + + const stall = () => { + setIsPending(false); + setHasStalled(true); + }; + + // A failing status check says nothing about the sync itself, so both the + // still-pending and the request-failed paths keep polling until the + // deadline rather than dropping the user into the next step early. + const keepPolling = () => { + if (Date.now() > deadline) { + stall(); + + return; + } + + setIsPending(true); + pollTimer = setTimeout(() => check(), POLL_INTERVAL_MS); + }; const check = async () => { try { - const { data } = await axios.get<{ pending: boolean }>( - syncStatus().url, - ); + const { data } = await axios.get<{ + pending: boolean; + failed: boolean; + }>(syncStatus().url, { timeout: POLL_INTERVAL_MS }); if (cancelled) { return; } - if (!data.pending) { + if (data.failed) { + stall(); + } else if (data.pending) { + keepPolling(); + } else { setIsPending(false); advance(); - } else { - setIsPending(true); - pollTimer = setTimeout(() => check(), 3000); } } catch { if (!cancelled) { - // On error, advance anyway to not block the user - advance(); + keepPolling(); } } }; @@ -87,6 +118,22 @@ export function StepSyncing({ onComplete }: StepSyncingProps) { return () => clearInterval(interval); }, [isPending]); + if (hasStalled) { + return ( +
+ + +
+ ); + } + // Don't render anything until we know sync is pending if (!isPending) { return null; diff --git a/tests/Feature/Onboarding/OnboardingSyncStatusTest.php b/tests/Feature/Onboarding/OnboardingSyncStatusTest.php index c76051ed..ee75e623 100644 --- a/tests/Feature/Onboarding/OnboardingSyncStatusTest.php +++ b/tests/Feature/Onboarding/OnboardingSyncStatusTest.php @@ -54,6 +54,46 @@ it('returns pending false when unsynced connection has an error status', functio ->assertJson(['pending' => false]); }); +it('reports a rate limited connection as failed instead of pending', function () { + $user = User::factory()->create(['onboarded_at' => null]); + + BankingConnection::factory()->for($user)->rateLimited()->create(); + + $this->actingAs($user) + ->getJson('/onboarding/sync-status') + ->assertOk() + ->assertJson(['pending' => false, 'failed' => true]); +}); + +it('keeps waiting while a healthy connection syncs alongside a failed one', function () { + $user = User::factory()->create(['onboarded_at' => null]); + + BankingConnection::factory()->for($user)->rateLimited()->create(); + BankingConnection::factory()->for($user)->create([ + 'status' => BankingConnectionStatus::Active, + 'last_synced_at' => null, + ]); + + $this->actingAs($user) + ->getJson('/onboarding/sync-status') + ->assertOk() + ->assertJson(['pending' => true, 'failed' => false]); +}); + +it('does not report a synced connection as failed', function () { + $user = User::factory()->create(['onboarded_at' => null]); + + BankingConnection::factory()->for($user)->create([ + 'status' => BankingConnectionStatus::Active, + 'last_synced_at' => now(), + ]); + + $this->actingAs($user) + ->getJson('/onboarding/sync-status') + ->assertOk() + ->assertJson(['pending' => false, 'failed' => false]); +}); + it('returns pending false when unsynced connection is revoked', function () { $user = User::factory()->create(['onboarded_at' => null]);