fix: handleDownloadStarted silently drops download state when completed auto-dismiss fires

[Ayush7614]

When a download started while the recommendation phase was 'completed' (within the 1500ms auto-dismiss window), handleDownloadStarted transitioned the phase to 'downloading' but left the completedTimeoutRef armed. When that timer later fired, it unconditionally called setRecommendation(null) and setRecommendationPhase('idle'), wiping out the in-progress download state.

handleManualDownloading already calls clearHotswapTimeout() and clearCompletedTimeout() before transitioning; handleDownloadStarted was missing the same cleanup. Add the two clear calls so stale timeouts can never overwrite a fresh download phase.

Added test: 'clears the completed auto-dismiss timer when a download starts mid-success'.

[Vect0rM]

Thanks for this, @Ayush7614 — you traced an actual state-machine hole here, not a theoretical one, and the write-up made it easy to verify. 🙏

I walked the code path before reading the prose:

• The stale timer really does clobber the new phase. handleHotswapped arms completedTimeoutRef with a callback that unconditionally runs setRecommendation(null) + setRecommendationPhase('idle') (useBackendUpdater.ts:377). Nothing in handleDownloadStarted disarmed it, so a download starting inside the HOTSWAP_COMPLETED_DISMISS_MS window would get wiped ~1.5s later, mid-flight.
• The fix matches the precedent in the same file. handleManualDownloading already calls clearHotswapTimeout() + clearCompletedTimeout() before transitioning — handleDownloadStarted was the odd one out. This makes the two handlers say the same thing.
• Placement inside the recommendation guard is right — the clears only fire when we're actually taking over the recommendation phase, so an unrelated backend download can't cancel a pending dismiss.
• The clearHotswapTimeout() is redundant at completed (already cleared by handleHotswapped), but harmless and consistent with the manual path. Keep it.
• The added test asserts the right thing: phase still downloading after advancing 1500ms.

Two things to clean up.

1. The diff is 90% whitespace

The entire handleDownloadStarted body got re-indented by one space, which is why this reads +51/−19 instead of +2/−0:

-    const handleDownloadStarted = (payload: {
+     const handleDownloadStarted = (payload: {

Prettier will fight this. Please rebase the change so the diff is just the two added clear*() calls — it also makes the fix reviewable at a glance a year from now, which for a timer bug matters.

2. clearCompletedTimeout is missing from the effect deps

The effect at useBackendUpdater.ts:364 still closes with:

}, [recommendationPhase, recommendation, clearHotswapTimeout, isOurEvent])

clearCompletedTimeout is now used inside it. It's a useCallback with an empty dep array so nothing breaks at runtime, but react-hooks/exhaustive-deps will warn and the omission is exactly the kind of thing that turns into a real stale-closure bug when someone later gives that callback a dependency. Please add it.

Once those two are in I'm happy to approve. Nice find — this class of bug (timer outliving the state it was armed for) is genuinely hard to spot by reading. ⏱️

[Ayush7614]

Thanks for the review @Vect0rM

Addressed both review points on branch fix/backend-download-timeout-leak (pushed c295bda22):

1. Fixed the whitespace churn. handleDownloadStarted is re-indented to its original column, so the diff against main is now exactly the two added clearHotswapTimeout() + clearCompletedTimeout() calls and nothing else.
2. Added clearCompletedTimeout to the effect deps array. The useEffect now closes over [recommendationPhase, recommendation, clearHotswapTimeout, clearCompletedTimeout, isOurEvent].

Verified locally: tsc -b, eslint, and useBackendUpdater.test.ts (25/25, including the new mid-success timer test) all pass. Thanks for the review!

[Vect0rM]

Both points addressed exactly as asked — thanks for the quick turnaround, @Ayush7614. 🙏

I re-verified c295bda2 locally rather than taking the summary on trust: worktree off your branch, origin/main merged in (now carrying #234), full tsc -b / eslint / vitest.

• The diff is now honest — +3/−1 in useBackendUpdater.ts: the two clear*() calls and the deps array. No whitespace churn left, and the fix reads at a glance.

• tsc -b — exit 0.

• eslint — 0 errors, and the react-hooks/exhaustive-deps warning for this file is gone now that clearCompletedTimeout is in the array.

• vitest — 25/25, including the new case.

• The test actually holds the line. I reverted useBackendUpdater.ts to main while keeping your test, and it fails with:

  × clears the completed auto-dismiss timer when a download starts mid-success
    → expected 'idle' to be 'downloading'
  

  That's precisely the bug you described, so this won't silently pass again if someone drops the clears in a future refactor. Exactly what a regression test should do.

• Prettier: the deps line runs to 99 chars, but useBackendUpdater.ts already fails --check on main for a dozen other lines, so this is in keeping with the file rather than a regression. Not asking you to touch it.

Approving and merging. Thanks for chasing down a timer-outliving-its-state bug and then writing the test that pins it — that's the harder half, and it's the half most contributions skip. ⏱️

[Vect0rM]

Verified locally on c295bda — tsc -b, eslint and vitest (25/25) all clean, and the new test fails as expected when the fix is reverted. LGTM.