fix: prevent infinite Manager loading offline and surface backend security messages#12752
Open
Kosinkadink wants to merge 2 commits into
Open
fix: prevent infinite Manager loading offline and surface backend security messages#12752Kosinkadink wants to merge 2 commits into
Kosinkadink wants to merge 2 commits into
Conversation
…urity messages - Wrap registry search in try/catch/finally so a failed/offline search clears the loading spinner instead of hanging forever, and expose error + retry - Add request timeouts to the registry and manager axios clients so hung sockets reject - Surface ComfyUI-Manager's actionable backend error message (e.g. required security_level/--listen) instead of a generic 403 fallback - Show a retryable connection-error empty state in the manager dialog Amp-Thread-ID: https://ampcode.com/threads/T-019eafaf-ac38-734b-8fa1-1422ed378e78 Co-authored-by: Amp <amp@ampcode.com>
dosubot
Bot
added
the
size:L
Contributor
📝 WalkthroughWalkthroughTracks registry search failures, exposes a retry action, surfaces errors in the Manager UI, adds 10s request timeouts to registry/manager clients, prefers backend-provided error messages, and exposes task-failure helpers for UI error rendering. ChangesSearch Error Recovery and API Reliability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 1662 passed, 0 failed · 3 flaky📊 Browser Reports
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workbench/extensions/manager/components/manager/ManagerDialog.vue`:
- Around line 487-493: The isLoading computed currently short-circuits on
searchError before checking isTabLoading, causing a stale search error to hide
the tab loading skeleton; update the isLoading logic in the isLoading computed
to check isTabLoading (and return true) before short-circuiting on searchError
so tab loading takes precedence, while preserving the existing checks for
isSearchLoading (with searchResults.length === 0) and isInitialLoad.
In `@src/workbench/extensions/manager/services/comfyManagerService.ts`:
- Around line 82-94: The current error-handling branch in comfyManagerService
that builds the user message (where axiosError, backendMessage,
routeSpecificErrors, status and context are used) doesn't treat axios timeouts
as cancellations; add an explicit branch before the generic fallback to detect
axios timeout errors (e.g., axiosError.code === 'ECONNABORTED' or
axiosError.message contains 'timeout') and set message to a clear user-facing
i18n string (use the existing i18n/t helper used elsewhere, e.g.,
t('comfyManager.timeout') or similar) so timeouts produce a specific translated
message instead of the generic `${context} failed with status ${status}`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: febe19dd-f55c-48e9-b336-95905f4b313d
📒 Files selected for processing (7)
src/locales/en/main.jsonsrc/services/comfyRegistryService.tssrc/workbench/extensions/manager/components/manager/ManagerDialog.vuesrc/workbench/extensions/manager/composables/useRegistrySearch.test.tssrc/workbench/extensions/manager/composables/useRegistrySearch.tssrc/workbench/extensions/manager/services/comfyManagerService.test.tssrc/workbench/extensions/manager/services/comfyManagerService.ts
Comment on lines
487
to
493
| const isLoading = computed(() => { | ||
| // A failed search must not read as "still loading" -- otherwise the spinner | ||
| // runs forever (e.g. offline) instead of showing the error placeholder. | ||
| if (searchError.value) return false | ||
| if (isSearchLoading.value) return searchResults.value.length === 0 | ||
| if (isTabLoading.value) return true | ||
| return isInitialLoad.value |
Contributor
There was a problem hiding this comment.
Prioritize tab loading before search-error short-circuiting.
On Line 490, searchError returns false before checking isTabLoading, so a stale search failure can suppress the loading skeleton while tab data is still loading.
Suggested fix
const isLoading = computed(() => {
- // A failed search must not read as "still loading" -- otherwise the spinner
- // runs forever (e.g. offline) instead of showing the error placeholder.
- if (searchError.value) return false
- if (isSearchLoading.value) return searchResults.value.length === 0
if (isTabLoading.value) return true
+ // A failed search must not read as "still loading" -- otherwise the spinner
+ // runs forever (e.g. offline) instead of showing the error placeholder.
+ if (searchError.value) return false
+ if (isSearchLoading.value) return searchResults.value.length === 0
return isInitialLoad.value
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isLoading = computed(() => { | |
| // A failed search must not read as "still loading" -- otherwise the spinner | |
| // runs forever (e.g. offline) instead of showing the error placeholder. | |
| if (searchError.value) return false | |
| if (isSearchLoading.value) return searchResults.value.length === 0 | |
| if (isTabLoading.value) return true | |
| return isInitialLoad.value | |
| const isLoading = computed(() => { | |
| if (isTabLoading.value) return true | |
| // A failed search must not read as "still loading" -- otherwise the spinner | |
| // runs forever (e.g. offline) instead of showing the error placeholder. | |
| if (searchError.value) return false | |
| if (isSearchLoading.value) return searchResults.value.length === 0 | |
| return isInitialLoad.value | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workbench/extensions/manager/components/manager/ManagerDialog.vue` around
lines 487 - 493, The isLoading computed currently short-circuits on searchError
before checking isTabLoading, causing a stale search error to hide the tab
loading skeleton; update the isLoading logic in the isLoading computed to check
isTabLoading (and return true) before short-circuiting on searchError so tab
loading takes precedence, while preserving the existing checks for
isSearchLoading (with searchResults.length === 0) and isInitialLoad.
Comment on lines
+82
to
94
| const backendMessage = axiosError.response?.data?.message | ||
| // Prefer the backend's message: ComfyUI-Manager returns actionable, | ||
| // security-aware text (e.g. which security_level/--listen is required) | ||
| // that is far more useful than our generic per-status fallbacks. | ||
| if (backendMessage) { | ||
| message = backendMessage | ||
| } else if (status && routeSpecificErrors?.[status]) { | ||
| message = routeSpecificErrors[status] | ||
| } else if (status === 404) { | ||
| message = 'Could not connect to ComfyUI-Manager' | ||
| } else { | ||
| message = | ||
| axiosError.response?.data?.message ?? | ||
| `${context} failed with status ${status}` | ||
| message = `${context} failed with status ${status}` | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that isAbortError handles axios timeout errors (ECONNABORTED)
# Check the implementation of isAbortError
ast-grep --pattern $'export $_isAbortError($_) {
$$$
}'
# Also search for ECONNABORTED or timeout handling
rg -n -C3 'ECONNABORTED|timeout.*error|isAbortError' --type=ts -g '!*.test.ts'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 9114
Handle axios timeout errors (ECONNABORTED) explicitly in comfyManagerService
isAbortError only matches DOMException AbortError, so axios timeouts (ECONNABORTED) won’t be treated as cancellations and will fall into the generic fallback (${context} failed with status ${status}), which can produce an unhelpful message (e.g., status being undefined). Add an ECONNABORTED/timeout branch in the error handler with a clear, user-facing (vue-i18n) message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workbench/extensions/manager/services/comfyManagerService.ts` around
lines 82 - 94, The current error-handling branch in comfyManagerService that
builds the user message (where axiosError, backendMessage, routeSpecificErrors,
status and context are used) doesn't treat axios timeouts as cancellations; add
an explicit branch before the generic fallback to detect axios timeout errors
(e.g., axiosError.code === 'ECONNABORTED' or axiosError.message contains
'timeout') and set message to a clear user-facing i18n string (use the existing
i18n/t helper used elsewhere, e.g., t('comfyManager.timeout') or similar) so
timeouts produce a specific translated message instead of the generic
`${context} failed with status ${status}`.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #12752 +/- ##
===========================================
- Coverage 75.39% 61.61% -13.78%
===========================================
Files 1560 1449 -111
Lines 89109 74916 -14193
Branches 24759 21129 -3630
===========================================
- Hits 67180 46163 -21017
- Misses 21256 28401 +7145
+ Partials 673 352 -321
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1119 files with indirect coverage changes 🚀 New features to boost your workflow:
|
When an install is rejected (e.g. ComfyUI-Manager blocks it due to the security_level/--listen restriction), the reason is captured in task history but the progress toast only rendered the streamed server logs -- which stay empty for a request rejected before the task runs. The failed task also misleadingly showed 'Completed'. - Expose isTaskFailed and getTaskErrorMessages from the manager store - Render a task's error messages in its failed panel - Label failed tasks 'Failed' (in danger color) instead of 'Completed' Amp-Thread-ID: https://ampcode.com/threads/T-019eafaf-ac38-734b-8fa1-1422ed378e78 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/workbench/extensions/manager/stores/comfyManagerStore.test.ts (1)
415-430: ⚡ Quick winPrefer function declaration for pure test helper.
The
historyItemhelper is a pure function with no side effects. Per coding guidelines, usefunction historyItem(...)instead ofconst historyItem = (...) =>for better hoisting clarity and consistency with the repository's functional style.♻️ Refactor to function declaration
- const historyItem = ( + function historyItem( uiId: string, statusStr: 'success' | 'error' | 'skip', messages: string[] - ): TaskHistoryItem => ({ + ): TaskHistoryItem { + return { ui_id: uiId, client_id: 'client', kind: 'install', result: statusStr === 'success' ? 'success' : 'failed', timestamp: '2024-01-01T00:00:00Z', status: { status_str: statusStr, completed: statusStr === 'success', messages } - }) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workbench/extensions/manager/stores/comfyManagerStore.test.ts` around lines 415 - 430, The test helper historyItem is a pure function defined as a const arrow; change it to a named function declaration to follow project style and hoisting conventions: replace the const historyItem = (...) => { ... } with function historyItem(uiId: string, statusStr: 'success' | 'error' | 'skip', messages: string[]): TaskHistoryItem { ... } and keep the same return shape (ui_id, client_id, kind, result, timestamp, status) and types to avoid changing behavior; ensure TaskHistoryItem typing is preserved and update any references to historyItem accordingly.Source: Coding guidelines
src/workbench/extensions/manager/components/ManagerProgressToast.vue (1)
44-54: ⚡ Quick winPrefer function declaration for consistency.
The
isTaskInProgresshelper is defined as an arrow function, but other helpers in this file (togglePanel,isAtBottom,scrollLastPanelToBottom) use function declarations. Per coding guidelines, preferfunction isTaskInProgress(taskId: string) { ... }for consistency and better hoisting clarity.♻️ Refactor to function declaration
-const isTaskInProgress = (taskId: string) => { +function isTaskInProgress(taskId: string) { const taskQueue = comfyManagerStore.taskQueue if (!taskQueue) return false const allQueueTasks = [ ...(taskQueue.running_queue || []), ...(taskQueue.pending_queue || []) ] return allQueueTasks.some((task) => task.ui_id === taskId) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workbench/extensions/manager/components/ManagerProgressToast.vue` around lines 44 - 54, Convert the isTaskInProgress arrow function into a function declaration to match other helpers; replace "const isTaskInProgress = (taskId: string) => { ... }" with "function isTaskInProgress(taskId: string) { ... }" while preserving the logic that reads comfyManagerStore.taskQueue, builds allQueueTasks from running_queue and pending_queue, and returns allQueueTasks.some(task => task.ui_id === taskId); keep the same null check for taskQueue and return false when absent.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/workbench/extensions/manager/components/ManagerProgressToast.vue`:
- Around line 44-54: Convert the isTaskInProgress arrow function into a function
declaration to match other helpers; replace "const isTaskInProgress = (taskId:
string) => { ... }" with "function isTaskInProgress(taskId: string) { ... }"
while preserving the logic that reads comfyManagerStore.taskQueue, builds
allQueueTasks from running_queue and pending_queue, and returns
allQueueTasks.some(task => task.ui_id === taskId); keep the same null check for
taskQueue and return false when absent.
In `@src/workbench/extensions/manager/stores/comfyManagerStore.test.ts`:
- Around line 415-430: The test helper historyItem is a pure function defined as
a const arrow; change it to a named function declaration to follow project style
and hoisting conventions: replace the const historyItem = (...) => { ... } with
function historyItem(uiId: string, statusStr: 'success' | 'error' | 'skip',
messages: string[]): TaskHistoryItem { ... } and keep the same return shape
(ui_id, client_id, kind, result, timestamp, status) and types to avoid changing
behavior; ensure TaskHistoryItem typing is preserved and update any references
to historyItem accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2db1d51e-78a8-4421-b99a-4868327c11bb
📒 Files selected for processing (3)
src/workbench/extensions/manager/components/ManagerProgressToast.vuesrc/workbench/extensions/manager/stores/comfyManagerStore.test.tssrc/workbench/extensions/manager/stores/comfyManagerStore.ts
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
1 participant
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
Frontend half of the ComfyUI-Manager Desktop fixes for Comfy-Org/Comfy-Desktop#1037. Companion to Comfy-Org/Comfy-Desktop#1041.
Fixes two Manager UX problems:
isLoading = truethen awaited the request with notry/finally, so any failure (offline, hung socket) never cleared the loading state.--listenis on andsecurity_levelis too strict), the UI showed a generic 403 message and discarded ComfyUI-Manager's actionable backend text describing exactly what is required.Changes
try/catch/finallyso a failed/offline search always clears the spinner; exposeerrorandretry.message(security-aware, actionable) over the generic per-status fallback.Testing
pnpm test:unitfor the newuseRegistrySearchandcomfyManagerServicespecs - pass.pnpm typecheck/pnpm lint/pnpm formatpass (run via pre-commit).Part of Comfy-Org/Comfy-Desktop#1037