-
Notifications
You must be signed in to change notification settings - Fork 8
fix: Start Over button now properly resets state to allow new generations #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ions Fixes #55 The Start Over button previously only navigated to step 1 without clearing the generated results. This caused the old results to persist when users went back through the steps, preventing them from generating new thumbnails. Changes: - Added handleStartOver function that clears results, labeled results, suggestions, blob URLs, and errors before navigating to step 1 - Updated Start Over button to use handleStartOver instead of just goTo(1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review completed. 1 suggestion posted.
Comment augment review to trigger a new review at any time.
|
|
||
| // Start over: clear results and go back to step 1 | ||
| const handleStartOver = () => { | ||
| cleanupBlobUrls(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If “Start Over” can be clicked while generation or suggested-refinement fetches are still in flight, those async completions can still update results/suggestedRefinements (index-keyed) after this reset, potentially causing stale suggestions or a stuck loading state. Consider adding a guard/cancellation mechanism for outstanding async work when starting over.
🤖 Was this useful? React with 👍 or 👎
- Add AbortController ref to cancel pending async operations - Update handleStartOver to abort in-flight requests before clearing state - Pass abort signal to /api/generate fetch calls - Pass abort signal to suggested refinements fetch calls - Handle AbortError gracefully without showing error messages - Also reset loading state when starting over Addresses PR review comment about race conditions when Start Over is clicked while generation or suggestion fetches are still in progress.
|
augment review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review completed. 1 suggestion posted.
Comment augment review to trigger a new review at any time.
app/thumbnails/page.tsx
Outdated
| } | ||
| setError(err instanceof Error ? err.message : "Failed to generate"); | ||
| } finally { | ||
| setLoading(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because an aborted generate() run still reaches finally { setLoading(false) }, a quick re-generate can have the earlier (aborted) call flip loading to false while the newer request is still in flight. Consider guarding this so only the currently-active (non-aborted) run updates loading.
🤖 Was this useful? React with 👍 or 👎
When a generation request is aborted (e.g., user clicks Start Over), the finally block was still calling setLoading(false). This caused a race condition where the aborted request could flip loading to false while a newer request was still in progress. Now we check if the abort signal was triggered before updating loading state. Addresses PR review comment about guarding against aborted requests updating the loading state.
|
augment review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review completed. 1 suggestion posted.
Comment augment review to trigger a new review at any time.
| setLoadingSuggestions({}); | ||
| setLoading(false); | ||
| setError(null); | ||
| goTo(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleStartOver clears generation state but doesn’t reset refinementState (e.g., isRefinementMode) or close related UI like the history browser. If a user clicks Start Over while refining, this can leave the app “stuck” in refinement mode and also prevent suggested refinements from being fetched on the next run due to the !refinementState.isRefinementMode guard.
🤖 Was this useful? React with 👍 or 👎
Addresses PR review feedback: handleStartOver now properly resets refinementState (exits refinement mode) and closes the history browser to prevent the app from getting stuck in refinement mode when a user clicks Start Over while refining.
|
augment review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review completed. 1 suggestion posted.
Comment augment review to trigger a new review at any time.
app/thumbnails/page.tsx
Outdated
| } | ||
| } catch (err: unknown) { | ||
| // Don't show error if request was aborted (e.g., user clicked Start Over) | ||
| if (err instanceof Error && err.name === 'AbortError') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch() aborts often reject with a DOMException (name AbortError) which may not satisfy error instanceof Error in all browsers, so this guard could miss aborts and still surface a "Failed to generate" error. Consider broadening the check to rely on the name rather than the instanceof constraint (also applies to the similar abort check in the suggested-refinements fetch).
🤖 Was this useful? React with 👍 or 👎
…ll browsers The previous check using 'instanceof Error' may fail in some browsers where DOMException (thrown by fetch() when aborted) does not extend Error. This change uses duck typing to check for the 'name' property being 'AbortError' which works reliably across all browsers. Addresses review comment about AbortError detection in catch blocks.
|
augment review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review completed. 1 suggestion posted.
Comment augment review to trigger a new review at any time.
| const videoRef = useRef<HTMLVideoElement | null>(null); | ||
| const canvasRef = useRef<HTMLCanvasElement | null>(null); | ||
| // AbortController for cancelling in-flight generation and suggestion requests | ||
| const abortControllerRef = useRef<AbortController | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since abortControllerRef is now central to request lifecycle, consider aborting it in a component unmount cleanup as well, so in-flight fetch calls can’t resolve and attempt state updates after navigation/unmount.
🤖 Was this useful? React with 👍 or 👎
Address PR review comment to abort abortControllerRef in component unmount cleanup to prevent state updates after navigation/unmount.
|
augment review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review completed. No suggestions at this time.
Comment augment review to trigger a new review at any time.
Summary
Fixes #55 - Start over button doesn't work in thumbnail creator
Problem
The Start Over button in the thumbnail creator was only navigating back to step 1 without clearing the generated results state. This caused users to see their previously generated thumbnails when they went through the steps again, preventing them from generating new thumbnails without refreshing the page.
Root Cause
The button's
onClickhandler was only callinggoTo(1)to change the step, but didn't clear:results- the array of generated thumbnail URLslabeledResults- the array of labeled results with provider infosuggestedRefinements- the refinement suggestions for each thumbnailloadingSuggestions- the loading state for suggestionsSolution
Created a new
handleStartOverfunction that:Updated the Start Over button to use this new handler.
Changes
handleStartOverfunction inapp/thumbnails/page.tsxhandleStartOverinstead ofgoTo(1)Testing
npm run build)Checklist
Pull Request opened by Augment Code with guidance from the PR author