Added download cancel/clear UI, fixed model downloading#238
Open
luminest-llc wants to merge 3 commits intojamiepine:mainfrom
Open
Added download cancel/clear UI, fixed model downloading#238luminest-llc wants to merge 3 commits intojamiepine:mainfrom
luminest-llc wants to merge 3 commits intojamiepine:mainfrom
Conversation
…ting - Add cancel (X) button on downloading and errored model items - Add collapsible Problems panel (VS Code-style) showing error details - Add "Clear All" button to reset all stale download/error state - Add POST /models/download/cancel endpoint to dismiss individual downloads - Add POST /tasks/clear endpoint to reset all task and progress state - Include error messages in /tasks/active response for visibility - Capture SSE error messages client-side for immediate display - Fix whisper-large using wrong HF repo (openai/whisper-large → openai/whisper-large-v3) - Fix Whisper HF repo mapping in both PyTorch and MLX backends - Shorten error toast to point users to Problems panel instead of wall of text
📝 WalkthroughWalkthroughAdds download cancellation and bulk-clear endpoints; frontend merges server activeTasks with local client errors, shows a Problems console with dismiss/clear controls, and exposes cancel/retry UI; ActiveDownloadTask gains optional error; Whisper repo IDs standardized to openai/whisper-large-v3. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant ApiClient
participant Backend
participant TaskMgr
participant ProgressMgr
User->>Frontend: Request model download
Frontend->>ApiClient: POST /models/download
ApiClient->>Backend: start download request
Backend->>TaskMgr: start_download(model_name)
TaskMgr-->>Backend: task registered
Backend-->>ApiClient: ack started
ApiClient-->>Frontend: started
Backend->>ProgressMgr: set progress (downloading)
ProgressMgr-->>Backend: persisted
alt Download error occurs
Backend->>ProgressMgr: set progress (error + message)
Backend->>TaskMgr: mark task errored
else User cancels download
User->>Frontend: Click Cancel
Frontend->>ApiClient: POST /models/download/cancel
ApiClient->>Backend: cancel_download(model_name)
Backend->>TaskMgr: cancel_download(model_name)
TaskMgr-->>Backend: cancelled
Backend->>ProgressMgr: clear progress
end
Frontend->>ApiClient: GET /tasks/active (poll)
ApiClient->>Backend: fetch tasks
Backend->>TaskMgr: get_active_downloads
Backend->>ProgressMgr: get_error_details
Backend-->>ApiClient: ActiveDownloadTask[] (includes error)
ApiClient-->>Frontend: updated tasks
sequenceDiagram
participant User
participant ModelItem
participant ModelMgmt
participant LocalState
User->>ModelItem: View model (errored)
ModelItem->>ModelMgmt: render request
ModelMgmt->>LocalState: merge server activeTasks + localErrors
LocalState-->>ModelMgmt: erroredDownloads
ModelMgmt->>ModelItem: props (erroredDownload, isDismissed, isCancelling)
ModelItem-->>User: show error badge + Retry/Cancel
alt User dismisses error
User->>ModelItem: Dismiss
ModelItem->>ModelMgmt: mark dismissed
ModelMgmt->>LocalState: add dismissedErrors
else User retries
User->>ModelItem: Retry
ModelItem->>ModelMgmt: trigger download
ModelMgmt->>LocalState: remove local error
else User cancels
User->>ModelItem: Cancel
ModelItem->>ModelMgmt: call onCancel
ModelMgmt->>ApiClient: cancelDownload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
backend/main.py (1)
1605-1618: Keep all progress-state clears under the same lock scope.
_last_notify_timeand_last_notify_progressare cleared outsideprogress_manager._lock, unlike_progress. Clear all three together under one lock for consistent state handling.♻️ Proposed refactor
- with progress_manager._lock: - progress_manager._progress.clear() - progress_manager._last_notify_time.clear() - progress_manager._last_notify_progress.clear() + with progress_manager._lock: + progress_manager._progress.clear() + progress_manager._last_notify_time.clear() + progress_manager._last_notify_progress.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 1605 - 1618, In clear_all_tasks(), progress_manager._last_notify_time and progress_manager._last_notify_progress are being cleared outside the progress_manager._lock while progress_manager._progress is cleared inside the lock; move the clear operations for _last_notify_time and _last_notify_progress into the same with progress_manager._lock scope so all three (_progress, _last_notify_time, _last_notify_progress) are cleared atomically under the lock to avoid race conditions.backend/utils/tasks.py (1)
75-80: Simplify cancellation withpopfor a single lookup path.This keeps behavior identical while reducing branching and dictionary lookups.
♻️ Proposed refactor
def cancel_download(self, model_name: str) -> bool: """Cancel/dismiss a download task (removes it from active list).""" - if model_name in self._active_downloads: - del self._active_downloads[model_name] - return True - return False + return self._active_downloads.pop(model_name, None) is not None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/tasks.py` around lines 75 - 80, The cancel_download method currently checks membership then deletes from _active_downloads; simplify it to a single dictionary lookup by using _active_downloads.pop(model_name, None) and return True if pop returned a value (or False otherwise), preserving behavior while reducing branching and double lookup in cancel_download.app/src/components/ServerSettings/ModelManagement.tsx (3)
159-170: Consider adding error handling for cancel mutation failure.The
handleCancelfunction updates local state optimistically before firing the mutation. IfcancelMutationfails, the UI shows the error as dismissed, but the backend state may not be updated. While the 5-second auto-refetch will eventually resync, users receive no feedback if the backend operation fails.For a more robust UX, consider adding an
onErrorhandler that could either rollback the optimistic update or show a toast notification.💡 Proposed enhancement
const cancelMutation = useMutation({ mutationFn: (modelName: string) => apiClient.cancelDownload(modelName), onSuccess: async () => { await queryClient.invalidateQueries({ queryKey: ['modelStatus'], refetchType: 'all' }); await queryClient.invalidateQueries({ queryKey: ['activeTasks'], refetchType: 'all' }); }, + onError: (error: Error) => { + toast({ + title: 'Failed to cancel download', + description: error.message, + variant: 'destructive', + }); + }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ServerSettings/ModelManagement.tsx` around lines 159 - 170, handleCancel performs optimistic local updates then calls cancelMutation.mutate without handling failures; add an onError callback to cancelMutation.mutate (or use cancelMutation.mutateAsync with try/catch) to either rollback the optimistic changes (restore setDismissedErrors and setLocalErrors and restore downloadingModel/downloadingDisplayName) or surface a user-facing error (e.g., show a toast) so failures are visible; modify the call site in handleCancel to pass an onError that receives the error and modelName and performs the rollback/toast logic using the same setters (setDismissedErrors, setLocalErrors, setDownloadingModel, setDownloadingDisplayName) to keep UI and backend consistent.
260-264:isCancellingis global, not per-model.The
isCancellingprop usescancelMutation.isPending, which reflects the global mutation state. If a user rapidly cancels multiple models, all cancel buttons will appear disabled simultaneously, regardless of which model is actually being cancelled.For more precise feedback, track cancellation state per model:
💡 Proposed enhancement using mutation variables
-isCancelling={cancelMutation.isPending} +isCancelling={cancelMutation.isPending && cancelMutation.variables === model.model_name}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ServerSettings/ModelManagement.tsx` around lines 260 - 264, The isCancelling prop is using the global cancelMutation.isPending which makes every model show cancelling state; change it to a per-model check by comparing the mutation's variables (or a dedicated state) to the current model name. Specifically, update the isCancelling prop logic so it checks something like cancelMutation.isPending && cancelMutation.variables?.<variableName> === model.model_name (or store the cancelling model id in a piece of state when handleCancel is called and clear it on completion) instead of using cancelMutation.isPending alone; adjust handleCancel and the mutation invocations accordingly to pass the model identifier so the comparison can be made.
312-316: Chevron icon direction appears inverted.The current logic shows
ChevronDownwhen the panel is open andChevronUpwhen closed. Standard accordion/collapsible UX typically uses:
ChevronDownwhen collapsed (pointing toward hidden content)ChevronUpwhen expanded (indicating "click to collapse")💡 Proposed fix
- {consoleOpen ? ( - <ChevronDown className="h-3.5 w-3.5" /> - ) : ( - <ChevronUp className="h-3.5 w-3.5" /> - )} + {consoleOpen ? ( + <ChevronUp className="h-3.5 w-3.5" /> + ) : ( + <ChevronDown className="h-3.5 w-3.5" /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ServerSettings/ModelManagement.tsx` around lines 312 - 316, The Chevron icons for the collapsible panel are inverted; currently, the code shows ChevronDown when open and ChevronUp when closed. In the JSX rendering the icons inside the component, switch the conditions so that ChevronDown is shown when consoleOpen is false (collapsed) and ChevronUp when consoleOpen is true (expanded), matching standard accordion UX conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/main.py`:
- Around line 1313-1320: The cache lookup in transcribe_audio builds model IDs
as "openai/whisper-{model_size}" which maps "large" to "openai/whisper-large"
while the rest of the code uses whisper_large_id = "openai/whisper-large-v3";
update transcribe_audio to use the same identifier mapping as the rest of the
module (e.g., reference the existing whisper_large_id variable or explicitly map
"large" to "openai/whisper-large-v3") so cache status/delete checks and
/transcribe use the identical model ID string and avoid false 202/downloading
results.
- Around line 1746-1757: The progress-only fallback path is not propagating
error details into the ActiveDownloadTask entries; when falling back to
progress_manager data retrieve the error from pm_data (e.g.,
pm_data.get("error")) and pass it into the ActiveDownloadTask constructor so
that ActiveDownloadTask.error is set (use the same local variable name like
error that you use in the task branch). Update the logic around
progress_manager._progress lookup and ActiveDownloadTask(...) creation so
pm_data error is assigned and forwarded.
- Around line 1595-1603: The variable removed from task_manager.cancel_download
is unused and the endpoint always returns "cancelled" even if no task was found;
update the handler to use the boolean/returned value from
task_manager.cancel_download (the removed flag) to decide whether to remove
progress state and to craft the response: only delete
progress_manager._progress[request.model_name] under the same
progress_manager._lock when removed is truthy, and return a 200 message like
"Download task for X cancelled" when removed is true, otherwise return a
404/appropriate message like "No download task for X found" (or an indicative
error response) so the response accurately reflects the cancellation result.
---
Nitpick comments:
In `@app/src/components/ServerSettings/ModelManagement.tsx`:
- Around line 159-170: handleCancel performs optimistic local updates then calls
cancelMutation.mutate without handling failures; add an onError callback to
cancelMutation.mutate (or use cancelMutation.mutateAsync with try/catch) to
either rollback the optimistic changes (restore setDismissedErrors and
setLocalErrors and restore downloadingModel/downloadingDisplayName) or surface a
user-facing error (e.g., show a toast) so failures are visible; modify the call
site in handleCancel to pass an onError that receives the error and modelName
and performs the rollback/toast logic using the same setters
(setDismissedErrors, setLocalErrors, setDownloadingModel,
setDownloadingDisplayName) to keep UI and backend consistent.
- Around line 260-264: The isCancelling prop is using the global
cancelMutation.isPending which makes every model show cancelling state; change
it to a per-model check by comparing the mutation's variables (or a dedicated
state) to the current model name. Specifically, update the isCancelling prop
logic so it checks something like cancelMutation.isPending &&
cancelMutation.variables?.<variableName> === model.model_name (or store the
cancelling model id in a piece of state when handleCancel is called and clear it
on completion) instead of using cancelMutation.isPending alone; adjust
handleCancel and the mutation invocations accordingly to pass the model
identifier so the comparison can be made.
- Around line 312-316: The Chevron icons for the collapsible panel are inverted;
currently, the code shows ChevronDown when open and ChevronUp when closed. In
the JSX rendering the icons inside the component, switch the conditions so that
ChevronDown is shown when consoleOpen is false (collapsed) and ChevronUp when
consoleOpen is true (expanded), matching standard accordion UX conventions.
In `@backend/main.py`:
- Around line 1605-1618: In clear_all_tasks(),
progress_manager._last_notify_time and progress_manager._last_notify_progress
are being cleared outside the progress_manager._lock while
progress_manager._progress is cleared inside the lock; move the clear operations
for _last_notify_time and _last_notify_progress into the same with
progress_manager._lock scope so all three (_progress, _last_notify_time,
_last_notify_progress) are cleared atomically under the lock to avoid race
conditions.
In `@backend/utils/tasks.py`:
- Around line 75-80: The cancel_download method currently checks membership then
deletes from _active_downloads; simplify it to a single dictionary lookup by
using _active_downloads.pop(model_name, None) and return True if pop returned a
value (or False otherwise), preserving behavior while reducing branching and
double lookup in cancel_download.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 696fa373-7044-4848-871c-c8edcfc7e1e1
📒 Files selected for processing (9)
app/src/components/ServerSettings/ModelManagement.tsxapp/src/lib/api/client.tsapp/src/lib/api/types.tsapp/src/lib/hooks/useModelDownloadToast.tsxbackend/backends/mlx_backend.pybackend/backends/pytorch_backend.pybackend/main.pybackend/models.pybackend/utils/tasks.py
- Fix transcribe_audio to use whisper-large-v3 mapping (not openai/whisper-large) - Propagate error field in progress-only fallback path for get_active_tasks - Use removed return value in cancel endpoint to vary response message - Add error rollback to handleCancel with toast on failure - Make isCancelling per-model instead of global - Fix inverted chevron icons in Problems panel - Move all clears under lock in clear_all_tasks - Simplify cancel_download to use dict.pop()
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/main.py (1)
1613-1627: Consider adding aclear()method toTaskManagerfor encapsulation.Directly accessing
task_manager._active_downloadsandtask_manager._active_generationsbypasses encapsulation. A dedicated method would be cleaner and would centralize any future synchronization needs.♻️ Suggested refactor
Add to
backend/utils/tasks.py:def clear_all(self) -> None: """Clear all active downloads and generations.""" self._active_downloads.clear() self._active_generations.clear()Then update the endpoint:
- task_manager._active_downloads.clear() - task_manager._active_generations.clear() + task_manager.clear_all()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 1613 - 1627, Add a clear_all(self) method to the TaskManager class (e.g., in backend/utils/tasks.py) that encapsulates clearing _active_downloads and _active_generations, then update the clear_all_tasks endpoint to call task_manager.clear_all() instead of directly touching task_manager._active_downloads and task_manager._active_generations; keep the existing progress_manager locking/clearing as-is (using progress_manager._lock, _progress, _last_notify_time, _last_notify_progress) unless you also add equivalent encapsulation methods for ProgressManager.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/main.py`:
- Around line 1613-1627: Add a clear_all(self) method to the TaskManager class
(e.g., in backend/utils/tasks.py) that encapsulates clearing _active_downloads
and _active_generations, then update the clear_all_tasks endpoint to call
task_manager.clear_all() instead of directly touching
task_manager._active_downloads and task_manager._active_generations; keep the
existing progress_manager locking/clearing as-is (using progress_manager._lock,
_progress, _last_notify_time, _last_notify_progress) unless you also add
equivalent encapsulation methods for ProgressManager.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbc5752a-10e9-4445-98e0-d5897b141bc6
📒 Files selected for processing (3)
app/src/components/ServerSettings/ModelManagement.tsxbackend/main.pybackend/utils/tasks.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/main.py (1)
1613-1626: Consider encapsulating progress clearing in ProgressManager.The endpoint directly accesses
progress_manager._progress,_last_notify_time, and_last_notify_progress. For consistency withTaskManager.clear_all(), consider adding aclear_all()method toProgressManagerthat encapsulates this logic.♻️ Suggested refactor
In
backend/utils/progress.py, add:def clear_all(self) -> None: """Clear all progress state.""" with self._lock: self._progress.clear() self._last_notify_time.clear() self._last_notify_progress.clear()Then in
backend/main.py:`@app.post`("/tasks/clear") async def clear_all_tasks(): """Clear all download tasks and progress state. Does not delete downloaded files.""" task_manager = get_task_manager() progress_manager = get_progress_manager() task_manager.clear_all() - - with progress_manager._lock: - progress_manager._progress.clear() - progress_manager._last_notify_time.clear() - progress_manager._last_notify_progress.clear() + progress_manager.clear_all() return {"message": "All task state cleared"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/main.py` around lines 1613 - 1626, The endpoint clear_all_tasks currently reaches into ProgressManager internals; add a public ProgressManager.clear_all(self) method that acquires self._lock and clears self._progress, self._last_notify_time, and self._last_notify_progress, then update the route to call progress_manager.clear_all() (keeping task_manager.clear_all() as is) by locating get_progress_manager() usage in clear_all_tasks and replacing the direct attribute manipulation with the new ProgressManager.clear_all() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/main.py`:
- Around line 1613-1626: The endpoint clear_all_tasks currently reaches into
ProgressManager internals; add a public ProgressManager.clear_all(self) method
that acquires self._lock and clears self._progress, self._last_notify_time, and
self._last_notify_progress, then update the route to call
progress_manager.clear_all() (keeping task_manager.clear_all() as is) by
locating get_progress_manager() usage in clear_all_tasks and replacing the
direct attribute manipulation with the new ProgressManager.clear_all() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3540528a-cd71-455b-9061-0aad2a92e65b
📒 Files selected for processing (2)
backend/main.pybackend/utils/tasks.py
|
voice box mein audio generate nahi kar paa raha hai |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements