fix: prevent infinite Manager loading offline and surface backend security messages#12752
fix: prevent infinite Manager loading offline and surface backend security messages#12752Kosinkadink wants to merge 3 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds shared API client timeouts, registry search retry handling, Manager dialog retry UI, manager service error remapping, task failure helpers in the store, and per-task error rendering in the progress toast. ChangesManager retry and error surfacing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
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
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>
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
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Reviewed via a multi-agent pass (13 reviewers) focused on how this fits the existing manager domain, the sibling services, and repo conventions/frameworks.
The two user-facing fixes are real and mostly land in the right layers (error/retry in the composable, normalization in the service, derived task state in the store, reuse of NoResultsPlaceholder + cn() + i18n + semantic tokens), and the core try/catch/finally spinner fix is correct and well tested.
Requesting changes on two points, both about alignment with the rest of the repo rather than the local logic:
-
comfyManagerService error handling now diverges from its sibling comfyRegistryService in two ways at once: the timeout this PR adds renders "failed with status undefined" on the offline/timeout case it targets (no no-response branch), and the backend-message precedence is inverted globally so curated/localized route and 404 messages are silently demoted for every status, not just the 403 security case in the description. See the two inline issues.
-
The timeout is copy-pasted into two of the three near-identical axios clients (the third, customerApiClient, is left with the original hung-socket bug). A shared client factory would fix all three and remove the duplicated constant/comment. Inline suggestion.
The rest are non-blocking suggestions and nitpicks on architecture shape, framework consistency, AGENTS.md comment style, a 'skip'-task edge case, and a11y/design polish. A rebase onto main (currently ~90 commits behind) before merge would also help. Happy to re-review once the two blocking items are addressed.
| // 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) { |
There was a problem hiding this comment.
issue: This reorders precedence so any backend data.message now wins over routeSpecificErrors[status] and the 404 -> "Could not connect to ComfyUI-Manager" string, for every route and status, not just the 403 security case the PR targets. Curated/localized messages (queueTask 404, updateAllPacks 401, the security_level strings) are silently demoted whenever the backend returns any body, and the status code is dropped from the fallback. The sibling comfyRegistryService.ts uses the opposite order (route-specific, then status switch, then data.message as the innermost default), so the two services that mirror each other now disagree on where an error string comes from. Could the backend-first rule be scoped to where it is needed, e.g. only when there is no routeSpecificErrors[status] match (or 403 only), so curated strings stay authoritative?
| message = | ||
| axiosError.response?.data?.message ?? | ||
| `${context} failed with status ${status}` | ||
| message = `${context} failed with status ${status}` |
There was a problem hiding this comment.
issue: The new client timeout is the headline fix, but a timeout/offline error is an axios error with response === undefined, so status and backendMessage are both undefined and this branch renders "<context> failed with status undefined", the exact vague UX the PR set out to remove, now reachable through the timeout it just added. The sibling comfyRegistryService.ts handles the no-response case (return ${context}: ${axiosError.message}); this service does not. Could you add a no-response / code === 'ECONNABORTED' branch before the final else, plus a timeout-shaped test (e.g. mockRejectedValue({ isAxiosError: true, code: 'ECONNABORTED' }))?
|
|
||
| // Without a timeout a hung socket (e.g. no internet, captive portal) never | ||
| // rejects, leaving callers stuck in their loading state indefinitely. | ||
| const REQUEST_TIMEOUT_MS = 10_000 |
There was a problem hiding this comment.
suggestion: There are three near-identical axios clients in the repo: managerApiClient here, registryApiClient in comfyRegistryService.ts, and customerApiClient in customerEventsService.ts. This PR adds the timeout to two of them but leaves customerApiClient without one, so the exact hung-socket bug this PR fixes stays latent there. Would a small shared createApiClient({ baseURL, ...overrides }) factory (baking in the JSON header + timeout, and later interceptors) be cleaner? It gives the timeout policy one home, lets customerApiClient inherit the fix for free, and removes the duplicated constant + comment, while each service still owns its baseURL and error mapping.
|
|
||
| const managerApiClient = axios.create({ | ||
| baseURL: api.apiURL('/v2/'), | ||
| timeout: REQUEST_TIMEOUT_MS, |
There was a problem hiding this comment.
question: This 10s timeout applies to every Manager endpoint on the shared client, including the synchronous POSTs that are not pure enqueues (REBOOT, UPDATE_COMFYUI, UPDATE_ALL, bulk LIST). On slow disks or large installs these can legitimately exceed 10s and would now abort where they previously succeeded. Would a higher ceiling on the shared client plus a per-request override for the search/status polls that actually motivated the fix be safer?
| QUEUE_TASK = 'manager/queue/task' | ||
| } | ||
|
|
||
| // Without a timeout a hung socket (e.g. no internet, captive portal) never |
There was a problem hiding this comment.
nitpick (non-blocking): AGENTS.md asks not to add multi-line block comments to justify trivial changes. The REQUEST_TIMEOUT_MS name already conveys intent, so this 2-line comment (duplicated verbatim in comfyRegistryService.ts) is the kind it discourages. Same for the two comment blocks above the small guards in ManagerDialog.vue (hasConnectionError, the isLoading early-return) and the // The bug: comment in useRegistrySearch.test.ts. Flagging for house-style alignment, not blocking.
| // restriction ComfyUI-Manager reports on a blocked install) lives in task | ||
| // history, not the streamed server logs -- which stay empty when the request | ||
| // is rejected before the task ever runs. Surface it so the failure isn't silent. | ||
| const getTaskErrorMessages = (taskId: string): string[] => |
There was a problem hiding this comment.
nitpick (non-blocking): Placement in the store is right (pure derivation over state the store owns). The smell is that getTaskErrorMessages reaches independently into taskHistory[taskId].status.messages, a third walk of task history alongside partitionTasks and partitionTaskLogs. Would building a failedTaskMessages map during the existing partition pass keep a single definition of "failed task plus its reason"?
|
|
||
| // The registry search failing (e.g. offline) is also a connection error worth | ||
| // surfacing, and unlike the manager-store error it can be retried in place. | ||
| const hasConnectionError = computed( |
There was a problem hiding this comment.
suggestion (non-blocking): hasConnectionError ORs two errors with different recovery semantics: the manager-store error is not retryable here, while searchError is. But @action always binds retrySearch() and the title/button-label are driven by different predicates, so a non-retryable manager error still shows a retry that re-runs the registry search. Modeling the empty-state error as one discriminated value ({ kind: 'search', retry } | { kind: 'manager' } | null) would let title/message/button/action derive from one source and never disagree. The if (searchError.value) return false guard in isLoading is a symptom of the same thing: the composable leaves loading/error mutual exclusion implicit rather than in the type.
| } = options | ||
|
|
||
| const isLoading = ref(false) | ||
| const error = ref<string | null>(null) |
There was a problem hiding this comment.
nitpick (non-blocking): This stores e.message (a string), but sibling composables in this domain (useNodePacks, useMissingNodes) expose the raw Error from useAsyncState. The only consumer here needs truthiness, so ref<Error | null> (assigning e directly and extracting the message at the display site) would match the domain's existing error shape and keep the type/cause for future consumers.
| v-else-if="displayPacks.length === 0" | ||
| :title="emptyStateTitle" | ||
| :message="emptyStateMessage" | ||
| :button-label="searchError ? $t('manager.retry') : undefined" |
There was a problem hiding this comment.
nitpick (non-blocking): Two small polish gaps on this connection-error state, both about matching existing patterns. (1) Design: the canonical error placeholder ErrorDialogContent.vue passes icon="pi pi-exclamation-circle", but this passes only title/message/button, so a hard connection failure renders icon-less, identical to a benign "no results" state. Could you pass an error icon when searchError is set? (2) A11y: this content swap happens in a plain div with no role="alert"/aria-live, so screen-reader users who triggered the search are not told it failed or that a retry exists. Wrapping the error branch in role="alert" would announce it. Both non-blocking.
| :key="`error-${errorIndex}`" | ||
| class="text-danger" | ||
| > | ||
| <pre class="wrap-break-word whitespace-pre-wrap">{{ |
There was a problem hiding this comment.
nitpick (non-blocking): a11y, the surfaced failure reason renders as <pre> lines with no aria-live, so the actionable security text (e.g. the security_level/--listen guidance) is not announced when it appears, and the log panel may be collapsed. The status label flipping to "Failed" is good (text, not color only); marking this container aria-live="polite" (or role="alert") would surface the reason to AT users.
- Scope backend-message preference to 403 so curated route/404 strings stay
authoritative (was overriding every status); keep the security goal intact.
- Handle timeout/offline (no-response) errors explicitly so they no longer
render "failed with status undefined".
- Check isTabLoading before short-circuiting on searchError in ManagerDialog so
a stale search error can't hide the tab loading skeleton.
- Use a generous shared timeout ceiling for long sync POSTs and a short
per-request timeout for reads/polls.
- Model the empty-state error as a discriminated value so a non-retryable
manager error no longer shows a search retry.
- Treat only status_str === 'error' as a failed task ('skip' is not a failure).
- Extract a shared createApiClient factory (JSON header + timeout), fixing the
latent missing timeout on customerApiClient.
- Add unit tests (timeout, route-string precedence, skipped task) and Playwright
regression coverage for the Manager offline/retry flow.
Amp-Thread-ID: https://ampcode.com/threads/T-019efced-7233-707c-bccf-7a4218671fdd
Co-authored-by: Amp <amp@ampcode.com>
2 participants
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