feat: replace GET /queue and /history calls with /jobs #7125
Conversation
📝 WalkthroughWalkthroughConsolidates history/queue handling into a unified Jobs API (JobListItem), removes legacy V1/V2 history adapters/types/fixtures/tests, adds jobs fetchers/types and reconcileJobs, updates composables/stores/components to use JobListItem, and introduces lazy-loading for full outputs and workflows. Changes
Sequence Diagram(s)(omitted) ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 12/04/2025, 10:03:13 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results❌ Some tests failed ⏰ Completed at: 12/04/2025, 10:13:01 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.2 MB (baseline 3.2 MB) • 🟢 -1.43 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 974 kB (baseline 973 kB) • 🔴 +1.33 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.54 kB (baseline 6.54 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 298 kB (baseline 298 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 173 kB (baseline 173 kB) • 🔴 +1 BReusable component library chunks
Status: 9 added / 9 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 3 added / 3 removed Utilities & Hooks — 2.94 kB (baseline 2.94 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 8.56 MB (baseline 8.56 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 5 added / 5 removed Other — 3.81 MB (baseline 3.81 MB) • ⚪ 0 BBundles that do not match a named category
Status: 23 added / 23 removed |
ric-yu
changed the title
ric-yu
force-pushed
the
ric-yu/use-jobs-api
branch
from
fc925a1 to
2f4f337
Compare
| // @ts-expect-error fixme ts strict error | ||
| ...(this.#reverse ? items[section].reverse() : items[section]).map( | ||
| (item: TaskItem) => { | ||
| (item: any) => { |
There was a problem hiding this comment.
this is legacy right? we don't need to strongly type this?
There was a problem hiding this comment.
True. You could use ts-expect-error
…ding Implements Jobs API endpoints (/jobs) for cloud distribution to replace history_v2 API, providing 99.998% memory reduction per item. Key changes: - Jobs API types, schemas, and fetchers for list and detail endpoints - Adapter to convert Jobs API format to TaskItem format - Lazy loading for full outputs when loading workflows - hasOnlyPreviewOutputs() detection for preview-only tasks - Feature flag to toggle between Jobs API and history_v2 Implementation details: - List endpoint: Returns preview_output only (100-200 bytes per job) - Detail endpoint: Returns full workflow and outputs on demand - Cloud builds use /jobs?status=completed for history view - Desktop builds unchanged (still use history_v1) - 21 unit and integration tests (all passing) Memory optimization: - Old: 300-600KB per history item (full outputs) - New: 100-200 bytes per history item (preview only) - Reduction: 99.998% Co-Authored-By: Claude <[email protected]>
ric-yu
force-pushed
the
ric-yu/use-jobs-api
branch
from
9915fa6 to
b733b88
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scripts/api.ts (1)
886-900: FixgetItems()method return type incompatibility — callinggetItems('history')will break the history UI in ui.ts.The
getHistory()return type change from object to array is a breaking change, but the issue extends beyond direct callers. ThegetItems()wrapper method (line 859–864 in api.ts) returns incompatible types:getItems('queue')returnsPromise<{Running: JobListItem[], Pending: JobListItem[]}>whilegetItems('history')returnsPromise<JobListItem[]>. The ui.ts code at lines 255–261 expects both to be objects with properties it can iterate over withObject.keys(items).flatMap((section) => items[section]). This will fail whengetItems('history')is called fromthis.history.load()at ui.ts line 253, as arrays don't have meaningful section properties.Update
getItems()to return a consistent structure for both queue and history types (e.g., wrap the history array to match the queue object structure or adjust ui.ts to handle both patterns).
🧹 Nitpick comments (10)
src/components/queue/job/JobDetailsPopover.stories.ts (1)
92-106: Consider removing unused_durationSecparameter.The
_durationSecparameter is not used in the function body. If it's no longer needed for the new JobListItem-based model, consider removing it to simplify the API. If kept for future use, a brief comment explaining why would help maintainability.function makeHistoryTask( id: string, priority: number, - _durationSec: number, ok: boolean, errorMessage?: string ): TaskItemImpl {Note: This would require updating all call sites (e.g., lines 178-181, 229-232, etc.).
tests-ui/tests/composables/useResultGallery.test.ts (1)
132-165: Consider verifying fetchApi was called in lazy loading test.The test validates that full outputs are loaded, but doesn't verify that
mockFetchApiwas actually invoked. Adding a spy or assertion would strengthen the test.+import { describe, it, expect, vi } from 'vitest' // ... it('loads full outputs when task has only preview outputs', async () => { // ... - const mockFetchApi = async () => new Response() + const mockFetchApi = vi.fn().mockResolvedValue(new Response()) const { galleryItems, galleryActiveIndex, onViewItem } = useResultGallery( () => [mockTask], mockFetchApi ) await onViewItem(createJobItem('job-1', previewOutput, mockTask)) expect(galleryItems.value).toEqual(fullOutputs) expect(galleryActiveIndex.value).toBe(0) + // Verify lazy loading was triggered (if fetchApi is called internally) + // expect(mockFetchApi).toHaveBeenCalled() })Note: Only add the assertion if
loadFullOutputsinternally uses the providedfetchApi. Based on learnings, use vitest mock functions over verbose manual mocks.src/scripts/ui.ts (1)
262-262: Consider using@ts-expect-errorinstead ofanyfor legacy code.As discussed in prior review comments, this legacy code path doesn't need strong typing. Using
@ts-expect-errorwould be more explicit about the intentional type relaxation while satisfying the coding guideline against usingany.- (item: any) => { + // @ts-expect-error Legacy UI code - items may be JobListItem or TaskItem + (item) => {src/composables/queue/useResultGallery.ts (1)
40-42: Cache lacks cleanup mechanism - potential memory leak.The
loadedTasksCacheMap grows unbounded as users view more items. Consider adding a cleanup strategy (e.g., LRU eviction, WeakMap if feasible, or clearing on certain events).+const MAX_CACHE_SIZE = 100 + const loadedTasksCache = new Map<string, GalleryTask>() let currentRequestId = 0 + +const trimCache = () => { + if (loadedTasksCache.size > MAX_CACHE_SIZE) { + const keys = [...loadedTasksCache.keys()] + keys.slice(0, keys.length - MAX_CACHE_SIZE).forEach(k => loadedTasksCache.delete(k)) + } +}Then call
trimCache()after adding to the cache.src/composables/queue/useJobMenu.ts (1)
56-61: Consider adding user feedback for workflow loading failures.When
getJobWorkflowreturnsundefined, the functions silently return without feedback to the user. Consider showing a toast or notification to inform users when workflow data cannot be loaded.const getJobWorkflow = async ( jobId: string ): Promise<ComfyWorkflowJSON | undefined> => { const jobDetail = await fetchJobDetail((url) => api.fetchApi(url), jobId) - return extractWorkflow(jobDetail) + const workflow = extractWorkflow(jobDetail) + if (!workflow) { + console.warn(`Could not load workflow for job ${jobId}`) + } + return workflow }The callers could then optionally show user-facing feedback if needed.
tests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts (2)
100-105: Avoidas anytype assertion.Per coding guidelines,
as anyshould not be used. Consider creating a properly typed test fixture or using a more specific type assertion that matches the expectedJobDetailstructure.- it('should extract workflow from job detail', () => { - const result = extractWorkflow(mockJobDetailResponse as any) + it('should extract workflow from job detail', () => { + const result = extractWorkflow(mockJobDetailResponse as JobDetail)You would need to import
JobDetailfrom the types module and ensuremockJobDetailResponseconforms to the schema.
113-122: Sameas anyissue for test fixture.This test also uses
as anywhich should be avoided. Consider typing the test fixture properly.+import type { JobDetail } from '@/platform/remote/comfyui/jobs/types/jobTypes' + // Then in the test: - const result = extractWorkflow(jobWithoutWorkflow as any) + const result = extractWorkflow(jobWithoutWorkflow as JobDetail)tests-ui/tests/store/queueStore.test.ts (1)
38-45: Avoidany[]type for images parameter.Per coding guidelines,
anytype should not be used. Consider using a proper type for the images array.+import type { ResultItem } from '@/schemas/apiSchema' + const createTaskOutput = ( nodeId: string = 'node-1', - images: any[] = [] + images: ResultItem[] = [] ): TaskOutput => ({ [nodeId]: { images } })src/platform/remote/comfyui/jobs/types/jobTypes.ts (1)
73-82: Consider adding.passthrough()tozExecutionErrorfor consistency.Other schemas use
.passthrough()for forward compatibility. ThezExecutionErrorschema is strict, which could cause validation failures if the backend adds new fields to execution errors in the future.const zExecutionError = z.object({ node_id: z.string(), node_type: z.string(), executed: z.array(z.string()), exception_message: z.string(), exception_type: z.string(), traceback: z.array(z.string()), current_inputs: z.unknown(), current_outputs: z.unknown() -}) +}).passthrough()src/stores/queueStore.ts (1)
290-293: Update outdated comment reference.The comment references
getWorkflowFromHistorywhich has been replaced by theloadWorkflow()method (line 393). Update the comment to reflect the current implementation.Apply this diff:
- get workflow(): ComfyWorkflowJSON | undefined { - // Workflow is only available after lazy loading via getWorkflowFromHistory - return undefined - } + get workflow(): ComfyWorkflowJSON | undefined { + // Workflow is lazily loaded via loadWorkflow() and not cached in TaskItemImpl + return undefined + }
ric-yu
force-pushed
the
ric-yu/use-jobs-api
branch
from
a3e7516 to
19b31e5
Compare
dosubot
bot
added
the
size:XXL
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/stores/queueStore.ts (2)
421-457: Missing error handling inloadWorkflow.Similar to
loadFullOutputs, this method callsfetchJobDetailandapp.loadGraphDatawithout error handling. A network failure or invalid workflow data could cause unhandled exceptions.public async loadWorkflow(app: ComfyApp) { if (!this.isHistory) { return } - // Single fetch for both workflow and outputs - const jobDetail = await fetchJobDetail( - (url) => app.api.fetchApi(url), - this.promptId - ) + let jobDetail + try { + jobDetail = await fetchJobDetail( + (url) => app.api.fetchApi(url), + this.promptId + ) + } catch { + return + } const workflowData = extractWorkflow(jobDetail) if (!workflowData) { return } - await app.loadGraphData(toRaw(workflowData)) + try { + await app.loadGraphData(toRaw(workflowData)) + } catch { + return + }
526-570: Storeupdate()lacks error handling for API failures.If
api.getQueue()orapi.getHistory()throws, the error propagates uncaught. Consider catching and logging errors to prevent UI breakage while still settingisLoadingto false. As per coding guidelines, implement proper error handling.const update = async () => { isLoading.value = true try { const [queue, history] = await Promise.all([ api.getQueue(), api.getHistory(maxHistoryItems.value) ]) // ... rest of update logic + } catch (error) { + console.error('Failed to update queue:', error) } finally { isLoading.value = false } }src/components/queue/job/useJobErrorReporting.ts (1)
39-55: Type cast fromExecutionErrortoExecutionErrorWsMessageis unsafe and may cause runtime issues.
ExecutionErrorWsMessageextendszExecutionWsMessageBasewhich requiresprompt_id(string) andtimestamp(number) fields. However,ExecutionErrorfrom the Jobs API schema does not include these fields. The cast at line 45 (executionError as ExecutionErrorWsMessage) masks this incompatibility.If
dialog.showExecutionErrorDialogrelies onprompt_idortimestamp, this will silently passundefinedvalues at runtime, potentially causing unexpected behavior.Resolve by either:
- Updating
JobErrorDialogService.showExecutionErrorDialog()to acceptExecutionErrordirectly and handle the absence ofprompt_id/timestamp- Creating a mapping function to transform
ExecutionErrortoExecutionErrorWsMessagewith synthesizedprompt_id/timestampvalues
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/components/queue/job/useJobErrorReporting.ts(3 hunks)src/composables/queue/useJobList.ts(1 hunks)src/composables/queue/useJobMenu.ts(4 hunks)src/composables/queue/useResultGallery.ts(1 hunks)src/platform/remote/comfyui/jobs/fetchers/fetchJobs.ts(1 hunks)src/platform/remote/comfyui/jobs/types/jobTypes.ts(1 hunks)src/stores/queueStore.ts(11 hunks)tests-ui/tests/components/queue/useJobErrorReporting.test.ts(2 hunks)tests-ui/tests/composables/useJobList.test.ts(4 hunks)tests-ui/tests/composables/useJobMenu.test.ts(11 hunks)tests-ui/tests/composables/useResultGallery.test.ts(2 hunks)tests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts(2 hunks)tests-ui/tests/stores/queueStore.loadWorkflow.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (20)
**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{vue,ts,tsx}: Leverage VueUse functions for performance-enhancing utilities
Use vue-i18n in Composition API for any string literals and place new translation entries in src/locales/en/main.json
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tstests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tssrc/composables/queue/useResultGallery.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/components/queue/useJobErrorReporting.test.tssrc/composables/queue/useJobMenu.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursorrules)
Use es-toolkit for utility functions
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tstests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tssrc/composables/queue/useResultGallery.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/components/queue/useJobErrorReporting.test.tssrc/composables/queue/useJobMenu.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use TypeScript for type safety
**/*.{ts,tsx}: Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tstests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tssrc/composables/queue/useResultGallery.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/components/queue/useJobErrorReporting.test.tssrc/composables/queue/useJobMenu.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
**/*.{ts,tsx,js,vue}
📄 CodeRabbit inference engine (.cursorrules)
Implement proper error handling in components and services
**/*.{ts,tsx,js,vue}: Use 2-space indentation, single quotes, no semicolons, and maintain 80-character line width as configured in.prettierrc
Organize imports by sorting and grouping by plugin, and runpnpm formatbefore committing
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tstests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tssrc/composables/queue/useResultGallery.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/components/queue/useJobErrorReporting.test.tssrc/composables/queue/useJobMenu.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tssrc/composables/queue/useResultGallery.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tssrc/composables/queue/useJobMenu.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tssrc/composables/queue/useResultGallery.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tssrc/composables/queue/useJobMenu.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use camelCase for variable and setting names in TypeScript/Vue files
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tstests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tssrc/composables/queue/useResultGallery.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/components/queue/useJobErrorReporting.test.tssrc/composables/queue/useJobMenu.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,vue}: Useconst settingStore = useSettingStore()andsettingStore.get('Comfy.SomeSetting')to retrieve settings in TypeScript/Vue files
Useawait settingStore.set('Comfy.SomeSetting', newValue)to update settings in TypeScript/Vue files
Check server capabilities usingapi.serverSupportsFeature('feature_name')before using enhanced features
Useapi.getServerFeature('config_name', defaultValue)to retrieve server feature configurationEnforce ESLint rules for Vue + TypeScript including: no floating promises, no unused imports, and i18n raw text restrictions in templates
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tstests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tssrc/composables/queue/useResultGallery.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/components/queue/useJobErrorReporting.test.tssrc/composables/queue/useJobMenu.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Define dynamic setting defaults using runtime context with functions in settings configuration
UsedefaultsByInstallVersionproperty for gradual feature rollout based on version in settings configuration
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tstests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tssrc/composables/queue/useResultGallery.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/components/queue/useJobErrorReporting.test.tssrc/composables/queue/useJobMenu.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using @ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tssrc/composables/queue/useResultGallery.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tssrc/composables/queue/useJobMenu.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.tssrc/composables/queue/useResultGallery.tssrc/platform/remote/comfyui/jobs/types/jobTypes.tssrc/composables/queue/useJobMenu.tssrc/components/queue/job/useJobErrorReporting.tssrc/stores/queueStore.tssrc/composables/queue/useJobList.ts
tests-ui/**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
tests-ui/**/*.test.{js,ts,jsx,tsx}: Write tests for new features
Follow existing test patterns in the codebase
Use existing test utilities rather than writing custom utilities
Mock external dependencies in tests
Always prefer vitest mock functions over writing verbose manual mocks
Files:
tests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/components/queue/useJobErrorReporting.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
**/*.{test,spec}.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Unit and component tests should be located in
tests-ui/or co-located with components assrc/components/**/*.{test,spec}.ts; E2E tests should be inbrowser_tests/
Files:
tests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/components/queue/useJobErrorReporting.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
**/composables/use*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name composables in the format
useXyz.ts
Files:
tests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/composables/useResultGallery.test.ts
src/**/{services,composables}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/{services,composables}/**/*.{ts,tsx}: Useapi.apiURL()for backend endpoints instead of constructing URLs directly
Useapi.fileURL()for static file access instead of constructing URLs directly
Files:
src/composables/queue/useResultGallery.tssrc/composables/queue/useJobMenu.tssrc/composables/queue/useJobList.ts
src/**/{composables,components}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Clean up subscriptions in state management to prevent memory leaks
Files:
src/composables/queue/useResultGallery.tssrc/composables/queue/useJobMenu.tssrc/components/queue/job/useJobErrorReporting.tssrc/composables/queue/useJobList.ts
src/**/{components,composables}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Use vue-i18n for ALL user-facing strings by adding them to
src/locales/en/main.json
Files:
src/composables/queue/useResultGallery.tssrc/composables/queue/useJobMenu.tssrc/components/queue/job/useJobErrorReporting.tssrc/composables/queue/useJobList.ts
src/components/**/*.{vue,ts,js}
📄 CodeRabbit inference engine (src/components/CLAUDE.md)
src/components/**/*.{vue,ts,js}: Use existing VueUse composables (such as useElementHover) instead of manually managing event listeners
Use useIntersectionObserver for visibility detection instead of custom scroll handlers
Use vue-i18n for ALL UI strings
Files:
src/components/queue/job/useJobErrorReporting.ts
src/**/stores/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/stores/**/*.{ts,tsx}: Maintain clear public interfaces and restrict extension access in stores
Use TypeScript for type safety in state management stores
Files:
src/stores/queueStore.ts
**/stores/*Store.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name Pinia stores with the
*Store.tssuffix
Files:
src/stores/queueStore.ts
🧠 Learnings (15)
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
tests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Write tests for new features
Applied to files:
tests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/components/queue/useJobErrorReporting.test.tstests-ui/tests/composables/useResultGallery.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Use existing test utilities rather than writing custom utilities
Applied to files:
tests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Follow existing test patterns in the codebase
Applied to files:
tests-ui/tests/composables/useJobList.test.tstests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/composables/useResultGallery.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Mock external dependencies in tests
Applied to files:
tests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: tests-ui/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:48:03.270Z
Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Always prefer vitest mock functions over writing verbose manual mocks
Applied to files:
tests-ui/tests/composables/useJobMenu.test.tstests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
📚 Learning: 2025-11-24T19:48:23.088Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:48:23.088Z
Learning: Use Vitest (with happy-dom) for unit and component tests, and Playwright for E2E tests
Applied to files:
tests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Use `vitest` for unit testing in this project
Applied to files:
tests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Use `test` instead of `it` for defining test cases in vitest
Applied to files:
tests-ui/tests/stores/queueStore.loadWorkflow.test.tstests-ui/tests/composables/useResultGallery.test.tstests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Prefer the use of `test.extend` over loose variables; import `test as baseTest` from `vitest`
Applied to files:
tests-ui/tests/stores/queueStore.loadWorkflow.test.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup
Applied to files:
tests-ui/tests/composables/useResultGallery.test.ts
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Use Playwright fixtures for browser tests
Applied to files:
tests-ui/tests/composables/useResultGallery.test.ts
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test across multiple viewports
Applied to files:
tests-ui/tests/composables/useResultGallery.test.ts
📚 Learning: 2025-11-24T19:48:23.088Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:48:23.088Z
Learning: Source code should be organized in `src/` directory with key areas: `components/`, `views/`, `stores/` (Pinia), `composables/`, `services/`, `utils/`, `assets/`, `locales/`
Applied to files:
src/stores/queueStore.ts
📚 Learning: 2025-11-24T19:47:14.779Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:14.779Z
Learning: Applies to **/*.{ts,tsx,vue} : Use `const settingStore = useSettingStore()` and `settingStore.get('Comfy.SomeSetting')` to retrieve settings in TypeScript/Vue files
Applied to files:
src/stores/queueStore.ts
🧬 Code graph analysis (8)
tests-ui/tests/composables/useJobMenu.test.ts (1)
src/stores/queueStore.ts (2)
workflow(331-333)executionError(317-319)
src/composables/queue/useResultGallery.ts (2)
src/stores/queueStore.ts (4)
url(84-86)ResultItemImpl(35-209)TaskItemImpl(211-496)outputsCount(296-298)src/composables/queue/useJobList.ts (1)
JobListItem(33-47)
tests-ui/tests/stores/queueStore.loadWorkflow.test.ts (2)
src/platform/remote/comfyui/jobs/types/jobTypes.ts (2)
JobListItem(120-120)JobDetail(121-121)src/platform/remote/comfyui/jobs/index.ts (2)
JobListItem(14-14)JobDetail(14-14)
src/platform/remote/comfyui/jobs/types/jobTypes.ts (2)
src/schemas/apiSchema.ts (2)
resultItemType(13-13)zTaskOutput(154-154)src/composables/queue/useJobList.ts (1)
JobListItem(33-47)
tests-ui/tests/components/queue/useJobErrorReporting.test.ts (3)
src/stores/queueStore.ts (4)
promptId(292-294)errorMessage(310-312)executionError(317-319)createTime(345-347)src/platform/remote/comfyui/jobs/types/jobTypes.ts (1)
ExecutionError(50-50)src/components/queue/job/useJobErrorReporting.ts (2)
JobErrorDialogService(9-18)useJobErrorReporting(26-62)
src/composables/queue/useJobMenu.ts (5)
src/platform/workflow/validation/schemas/workflowSchema.ts (1)
ComfyWorkflowJSON(460-462)src/platform/remote/comfyui/jobs/fetchers/fetchJobs.ts (2)
fetchJobDetail(127-144)extractWorkflow(150-160)src/platform/remote/comfyui/jobs/index.ts (2)
fetchJobDetail(11-11)extractWorkflow(9-9)src/scripts/api.ts (1)
api(1247-1247)src/services/dialogService.ts (1)
useDialogService(47-566)
src/components/queue/job/useJobErrorReporting.ts (2)
src/stores/queueStore.ts (1)
executionError(317-319)src/schemas/apiSchema.ts (1)
ExecutionErrorWsMessage(144-144)
tests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts (2)
src/platform/remote/comfyui/jobs/fetchers/fetchJobs.ts (2)
fetchJobDetail(127-144)extractWorkflow(150-160)src/platform/remote/comfyui/jobs/index.ts (2)
fetchJobDetail(11-11)extractWorkflow(9-9)
🔇 Additional comments (35)
src/platform/remote/comfyui/jobs/types/jobTypes.ts (1)
1-121: Well-structured Zod schemas for the new Jobs API.The file establishes a clean separation between raw API data (
zRawJobListItem) and normalized data with required fields (zJobListItem). The use of.passthrough()on all schemas enables forward compatibility with new backend fields.A few observations:
The
zJobStatusschema is defined but not exported — if consumers need to validate status values independently, consider exporting it.Consider adding explicit return type for
ExecutionErrorto align withExecutionErrorWsMessagefrom@/schemas/apiSchema(referenced inuseJobErrorReporting.tsline 45 where a cast is used).src/stores/queueStore.ts (2)
459-479: LGTM:flatten()correctly creates synthetic task items.The method properly creates new
TaskItemImplinstances with unique synthetic IDs for each output, maintaining immutability.
211-232: LGTM: Constructor handles optional outputs gracefully.The constructor correctly creates synthetic outputs from
preview_outputwhen full outputs aren't available, and properly strips animated outputs.tests-ui/tests/components/queue/useJobErrorReporting.test.ts (1)
23-93: LGTM: Good test coverage for error reporting flows.Tests properly cover reactive error message updates, copy-to-clipboard behavior, empty states, and the dialog routing logic between simple and rich error dialogs.
src/composables/queue/useJobList.ts (1)
237-246: LGTM: Correctly updated to useworkflowIdproperty.The change from
task.workflow?.idtotask.workflowIdaligns with the newTaskItemImplAPI whereworkflowIdis a direct getter from the underlyingJobListItem.tests-ui/tests/composables/useJobList.test.ts (2)
9-17: LGTM: Test type correctly updated for new data model.The
TestTasktype now usesworkflowId?: stringdirectly instead ofworkflow?: { id?: string }, matching theJobListItemshape.
384-413: LGTM: Workflow filter test properly usesworkflowId.Test data correctly uses
workflowId: 'workflow-1'to test the active workflow filtering logic.src/composables/queue/useResultGallery.ts (1)
51-83: Good abort pattern for stale requests.The
currentRequestIdpattern (lines 55, 65) correctly prevents race conditions when rapid view changes occur. The logic properly aborts stale requests before updating gallery state.tests-ui/tests/composables/useResultGallery.test.ts (3)
8-23: Helper function correctly creates test fixtures with overridden getters.The
createResultItemhelper properly constructsResultItemImplinstances and usesObject.definePropertyto override getters for test matching. This is a valid approach for testing.
25-45: Test fixtures properly align with new JobListItem-based structure.The
createMockJobandcreateTaskhelpers correctly construct the new data shapes required by the Jobs API migration.
47-58: Type assertion acceptable for test fixture creation.The
as JobListViewItemassertion is appropriate here since the test only requires specific fields to be present.tests-ui/tests/stores/queueStore.loadWorkflow.test.ts (4)
29-46: Mock job detail structure correctly reflects the nested workflow path.The
mockJobDetailstructure withworkflow.extra_data.extra_pnginfo.workflowaccurately mirrors the actual/jobs/{id}API response structure as documented inextractWorkflow.
48-66: Helper functions correctly create JobListItem test fixtures.The
createHistoryJobandcreateRunningJobhelpers properly construct the minimal required fields for testing different job states.
86-101: Test correctly verifies lazy workflow loading flow.The test properly mocks
fetchJobDetail, verifies it's called with the job ID, and confirmsloadGraphDatareceives the expected workflow data.
115-127: Good coverage for running task exclusion.This test ensures that running/in-progress tasks do not trigger workflow fetching, which is the expected behavior since they don't have completed workflow data yet.
src/composables/queue/useJobMenu.ts (4)
53-61: Internal helper correctly implements lazy workflow loading.The
getJobWorkflowfunction properly encapsulates the fetch + extract pattern. Error handling is delegated tofetchJobDetailwhich returnsundefinedon failure.
90-94: Simplified error message access is cleaner.The refactored
copyErrorMessagedirectly accesseserrorMessagefromtaskRef, removing unnecessary parsing logic.
96-115: Error reporting logic correctly prioritizes rich dialog.The two-path approach properly handles both structured
execution_error(showing detailed dialog) and fallback to simple error dialog when only a message is available.
181-203: Export workflow correctly uses lazy loading.The
exportJobWorkflowfunction properly integrates withgetJobWorkflowfor on-demand workflow retrieval.tests-ui/tests/platform/workflow/cloud/getWorkflowFromHistory.test.ts (3)
19-41: Mock response structure correctly reflects actual API contract.The
mockJobDetailResponseaccurately represents the/jobs/{id}response with the nested workflow path atworkflow.extra_data.extra_pnginfo.workflow.
43-99: Comprehensive test coverage for fetchJobDetail.The tests properly verify endpoint construction, successful response parsing, 404 handling, network errors, and malformed JSON responses.
102-124: Good coverage for extractWorkflow edge cases.The tests properly verify extraction from valid data, handling of
undefinedinput, and graceful handling of missing nested workflow fields. Theas anyassertions are acceptable for testing malformed data scenarios.src/platform/remote/comfyui/jobs/fetchers/fetchJobs.ts (6)
1-18: Well-documented module with clear purpose.The file header clearly explains the module's role in the unified Jobs API architecture, and imports are properly organized.
34-54: Internal fetcher with proper error handling.The
fetchJobsRawfunction correctly handles both network errors and parse failures, returning safe empty results while logging for debugging.
63-90: Priority assignment logic correctly preserves server values.The
assignPriorityfunction uses nullish coalescing (??) to only assign synthetic priorities when the server doesn't provide one. The history priority calculationtotal - offsetensures proper ordering.
96-118: Queue priority logic correctly ensures pending > running > history.The priority assignment ensures:
- Pending jobs get highest priority (
QUEUE_PRIORITY_BASE + running.length + pending.length)- Running jobs are next (
QUEUE_PRIORITY_BASE + running.length)- Both are above any history job due to
QUEUE_PRIORITY_BASE
127-144: Job detail fetcher with graceful error handling.The
fetchJobDetailfunction properly handles non-OK responses and exceptions, returningundefinedto signal failure. Logging provides visibility into issues.
150-160: Workflow extraction with documented type assertions.The type assertions in
extractWorkfloware justified by the comment explaining downstream validation vialoadGraphData -> validateComfyWorkflow. The safe optional chaining prevents runtime errors on missing nested fields.tests-ui/tests/composables/useJobMenu.test.ts (7)
61-66: Mocks correctly set up for lazy loading workflow retrieval.The
fetchJobDetailMockandextractWorkflowMockare properly configured to test the new workflow loading path via the jobs module.
183-186: Default mock configuration requires explicit workflow setup.Setting defaults to
undefinedensures each test must explicitly configure workflow availability, preventing false positives.
192-211: Test correctly verifies lazy workflow loading integration.The test properly sets up mocks for the fetch + extract flow and verifies that
fetchJobDetailis called with the job ID before workflow processing.
291-325: Comprehensive test for execution_error rich dialog path.The test properly verifies that when
executionErroris present, the richshowExecutionErrorDialogis called with the full error payload, and the fallbackshowErrorDialogis not invoked.
327-347: Good coverage of fallback error dialog path.The test verifies that when only
errorMessageis available (noexecutionError), the simpleshowErrorDialogis called with an Error instance and the correctreportType.
540-563: Export workflow test correctly uses lazy loading mocks.The test properly configures
fetchJobDetailMockandextractWorkflowMockto supply workflow data, then verifies the export produces the correct JSON blob.
738-757: Failed menu test correctly uses new errorMessage field.The test properly sets up
taskRef.errorMessageto verify the failed job menu structure includes error-related actions.
| const loadedTasksCache = new Map<string, TaskItemImpl>() | ||
| let currentRequestId = 0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cache and request ID should be instance-scoped, not shared.
loadedTasksCache and currentRequestId are declared inside the function but using const and let makes them shared across component re-renders but not across instances. However, if useResultGallery is called multiple times (e.g., multiple gallery instances), each will have its own cache — this is actually correct.
The cache lacks a cleanup mechanism, which could lead to memory growth over time if many jobs are viewed.
Consider adding cache size limits or clearing on unmount:
+import { onUnmounted } from 'vue'
+
export function useResultGallery(
getFilteredTasks: () => TaskItemImpl[],
fetchApi?: FetchApi
) {
const galleryActiveIndex = ref(-1)
const galleryItems = shallowRef<ResultItemImpl[]>([])
const loadedTasksCache = new Map<string, TaskItemImpl>()
let currentRequestId = 0
+ onUnmounted(() => {
+ loadedTasksCache.clear()
+ })📝 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.
| const loadedTasksCache = new Map<string, TaskItemImpl>() | |
| let currentRequestId = 0 | |
| import { onUnmounted } from 'vue' | |
| export function useResultGallery( | |
| getFilteredTasks: () => TaskItemImpl[], | |
| fetchApi?: FetchApi | |
| ) { | |
| const galleryActiveIndex = ref(-1) | |
| const galleryItems = shallowRef<ResultItemImpl[]>([]) | |
| const loadedTasksCache = new Map<string, TaskItemImpl>() | |
| let currentRequestId = 0 | |
| onUnmounted(() => { | |
| loadedTasksCache.clear() | |
| }) |
🤖 Prompt for AI Agents
In src/composables/queue/useResultGallery.ts around lines 27-28, the
per-instance loadedTasksCache and currentRequestId lack a cleanup/size policy
and may grow unbounded; implement an instance-scoped cache eviction and
teardown: add a maxCacheSize constant and evict oldest entries when inserting
beyond the limit (or convert Map to an LRU by tracking access order), expose or
call a clearCache function when switching jobs, and register an onUnmounted hook
to clear the Map and reset currentRequestId so memory is released when the
component instance is destroyed.
There was a problem hiding this comment.
Cache and request ID should be instance-scoped, not shared.
loadedTasksCache and currentRequestId are declared inside the function but using const and let makes them shared across component re-renders but not across instances. However, if useResultGallery is called multiple times (e.g., multiple gallery instances), each will have its own cache — this is actually correct.
This is just LLM nonsense right
There was a problem hiding this comment.
@ric-yu, you're absolutely right. My explanation of the scoping was confusing and contradictory.
The scoping here is actually correct and intentional: each call to useResultGallery() creates its own closure with a dedicated loadedTasksCache Map and currentRequestId counter. Within a single component instance, these persist across re-renders (which is exactly what you want for caching), and different component instances each get their own independent cache.
The memory growth concern about unbounded cache might still apply if a single gallery instance views hundreds of different jobs over its lifetime, but that's a separate issue from scoping. The current implementation is fine for typical usage patterns.
Sorry for the noise—the original comment should have focused solely on cache eviction policy if at all, not scope.
There was a problem hiding this comment.
When component unmounts, the closure (including the Map) gets garbage collected anyway. Explicitly clearing it seems redundant.
There was a problem hiding this comment.
@ric-yu, you're absolutely correct. When the component unmounts and there are no external references to the composable's closure, the entire closure—including the Map—becomes eligible for garbage collection automatically. JavaScript applications need to ensure garbage collection takes place as expected, but in this case, the Map has no external side effects or listeners that would prevent collection.
Explicit cleanup with onUnmounted is essential for resources with external references, such as:
- Event listeners that need
removeEventListenercalls - Timers created with
setIntervalorsetTimeout - Third-party library instances with
.destroy()methods
But for a plain Map with no external bindings, as you noted, the GC will handle it. My original suggestion to add explicit cleanup was unnecessary—apologies for the noise.
🧠 Learnings used
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Applies to src/**/{composables,components}/**/*.{ts,tsx,vue} : Clean up subscriptions in state management to prevent memory leaks
|
Will address coderabbit comments and then re-open |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/queue/job/JobGroupsList.vue (1)
61-91: Add cleanup for hover detail timers (and consider VueUse helper)
showTimer/hideTimertimeouts are cleared on enter/leave, but not on component unmount. To fully comply with the “cleanup async operations” guideline and avoid stray callbacks after unmount, consider:
- Clearing both timers in an
onBeforeUnmounthook, and/or- Replacing the manual
setTimeoutlogic with a VueUse helper likeuseTimeoutFn, which auto‑cleans on unmount.Example sketch:
-import { ref } from 'vue' +import { ref, onBeforeUnmount } from 'vue' // ... const clearHideTimer = () => { if (hideTimer.value !== null) { clearTimeout(hideTimer.value) hideTimer.value = null } } const clearShowTimer = () => { if (showTimer.value !== null) { clearTimeout(showTimer.value) showTimer.value = null } } +onBeforeUnmount(() => { + clearHideTimer() + clearShowTimer() +})Or wrap this behavior with VueUse’s timeout utilities for simpler, auto‑cleaned timers.
src/components/queue/job/JobDetailsPopover.stories.ts (1)
150-161: Useas constassertions or explicit types to avoidanycasts.Lines 160-161 use
as anyforexec.nodeProgressStatesByPrompt. Per coding guidelines, avoidas anytype assertions. Consider defining a proper type for the progress state or using a more specific type assertion.exec.nodeProgressStatesByPrompt = { p1: { '1': { value: 1, max: 1, state: 'running', node_id: '1', prompt_id: 'p1' } } - } as any + } as Record<string, Record<string, { value: number; max: number; state: string; node_id: string; prompt_id: string }>>
♻️ Duplicate comments (1)
tests-ui/tests/composables/useResultGallery.test.ts (1)
141-169:mockFetchApiis unused — consider removing or documenting.The
mockFetchApi(line 158) is passed touseResultGallery, buttask.loadFullOutputsis directly mocked on line 155, so the composable'sfetchApiparameter is never invoked. Either removemockFetchApiif not needed, or add a comment explaining it's intentionally provided to satisfy the composable signature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/components/queue/job/JobDetailsPopover.stories.ts(3 hunks)src/components/queue/job/JobGroupsList.vue(1 hunks)src/platform/remote/comfyui/jobs/types/jobTypes.ts(1 hunks)src/stores/queueStore.ts(11 hunks)tests-ui/tests/composables/useResultGallery.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (24)
**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{vue,ts,tsx}: Leverage VueUse functions for performance-enhancing utilities
Use vue-i18n in Composition API for any string literals and place new translation entries in src/locales/en/main.json
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vuesrc/stores/queueStore.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursorrules)
Use es-toolkit for utility functions
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/stores/queueStore.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use TypeScript for type safety
**/*.{ts,tsx}: Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/stores/queueStore.ts
**/*.{ts,tsx,js,vue}
📄 CodeRabbit inference engine (.cursorrules)
Implement proper error handling in components and services
**/*.{ts,tsx,js,vue}: Use 2-space indentation, single quotes, no semicolons, and maintain 80-character line width as configured in.prettierrc
Organize imports by sorting and grouping by plugin, and runpnpm formatbefore committing
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vuesrc/stores/queueStore.ts
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vuesrc/stores/queueStore.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/stores/queueStore.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use camelCase for variable and setting names in TypeScript/Vue files
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vuesrc/stores/queueStore.ts
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,vue}: Useconst settingStore = useSettingStore()andsettingStore.get('Comfy.SomeSetting')to retrieve settings in TypeScript/Vue files
Useawait settingStore.set('Comfy.SomeSetting', newValue)to update settings in TypeScript/Vue files
Check server capabilities usingapi.serverSupportsFeature('feature_name')before using enhanced features
Useapi.getServerFeature('config_name', defaultValue)to retrieve server feature configurationEnforce ESLint rules for Vue + TypeScript including: no floating promises, no unused imports, and i18n raw text restrictions in templates
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vuesrc/stores/queueStore.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Define dynamic setting defaults using runtime context with functions in settings configuration
UsedefaultsByInstallVersionproperty for gradual feature rollout based on version in settings configuration
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tstests-ui/tests/composables/useResultGallery.test.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/stores/queueStore.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using @ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vuesrc/stores/queueStore.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/platform/remote/comfyui/jobs/types/jobTypes.tssrc/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vuesrc/stores/queueStore.ts
tests-ui/**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
tests-ui/**/*.test.{js,ts,jsx,tsx}: Write tests for new features
Follow existing test patterns in the codebase
Use existing test utilities rather than writing custom utilities
Mock external dependencies in tests
Always prefer vitest mock functions over writing verbose manual mocks
Files:
tests-ui/tests/composables/useResultGallery.test.ts
**/*.{test,spec}.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Unit and component tests should be located in
tests-ui/or co-located with components assrc/components/**/*.{test,spec}.ts; E2E tests should be inbrowser_tests/
Files:
tests-ui/tests/composables/useResultGallery.test.ts
**/composables/use*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name composables in the format
useXyz.ts
Files:
tests-ui/tests/composables/useResultGallery.test.ts
src/**/{composables,components}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Clean up subscriptions in state management to prevent memory leaks
Files:
src/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vue
src/**/{components,composables}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Use vue-i18n for ALL user-facing strings by adding them to
src/locales/en/main.json
Files:
src/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vue
src/components/**/*.{vue,ts,js}
📄 CodeRabbit inference engine (src/components/CLAUDE.md)
src/components/**/*.{vue,ts,js}: Use existing VueUse composables (such as useElementHover) instead of manually managing event listeners
Use useIntersectionObserver for visibility detection instead of custom scroll handlers
Use vue-i18n for ALL UI strings
Files:
src/components/queue/job/JobDetailsPopover.stories.tssrc/components/queue/job/JobGroupsList.vue
**/*.vue
📄 CodeRabbit inference engine (.cursorrules)
**/*.vue: Use setup() function for component logic in Vue 3 Composition API
Utilize ref and reactive for reactive state in Vue 3
Implement computed properties with computed() function
Use watch and watchEffect for side effects in Vue 3
Implement lifecycle hooks with onMounted, onUpdated, etc.
Utilize provide/inject for dependency injection in Vue 3
Use Vue 3.5 style of default prop declaration with defineProps()
Organize Vue components in <script> <style> order
Use Tailwind CSS for styling Vue components
Implement responsive design with Tailwind CSS
Do not use deprecated PrimeVue components (Dropdown, OverlayPanel, Calendar, InputSwitch, Sidebar, Chips, TabMenu, Steps, InlineMessage). Use replacements: Select, Popover, DatePicker, ToggleSwitch, Drawer, AutoComplete, Tabs, Stepper, Message respectively
Implement proper props and emits definitions in Vue components
Utilize Vue 3's Teleport component when needed
Use Suspense for async components in Vue 3
Follow Vue 3 style guide and naming conventions
Never use deprecated PrimeVue components (Dropdown, OverlayPanel, Calendar, InputSwitch, Sidebar, Chips, TabMenu, Steps, InlineMessage)Never use
:class="[]"to merge class names - always useimport { cn } from '@/utils/tailwindUtil'for class merging in Vue templates
**/*.vue: Use TypeScript with Vue 3 Single File Components (.vuefiles)
Name Vue components in PascalCase (e.g.,MenuHamburger.vue)Files:
src/components/queue/job/JobGroupsList.vuesrc/**/*.vue
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.vue: Use the Vue 3 Composition API instead of the Options API when writing Vue components (exception: when overriding or extending PrimeVue components for compatibility)
Use setup() function for component logic
Utilize ref and reactive for reactive state
Implement computed properties with computed()
Use watch and watchEffect for side effects
Implement lifecycle hooks with onMounted, onUpdated, etc.
Utilize provide/inject for dependency injection
Use vue 3.5 style of default prop declaration
Use Tailwind CSS for styling
Implement proper props and emits definitions
Utilize Vue 3's Teleport component when needed
Use Suspense for async components
Follow Vue 3 style guide and naming conventionsFiles:
src/components/queue/job/JobGroupsList.vue**/*.{vue,html}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
dark:ordark-theme:Tailwind variants - instead use semantic values fromstyle.csstheme, e.g.bg-node-component-surfaceFiles:
src/components/queue/job/JobGroupsList.vuesrc/components/**/*.vue
📄 CodeRabbit inference engine (src/components/CLAUDE.md)
src/components/**/*.vue: Use setup() function in Vue 3 Composition API
Destructure props using Vue 3.5 style in Vue components
Use ref/reactive for state management in Vue 3 Composition API
Implement computed() for derived state in Vue 3 Composition API
Use provide/inject for dependency injection in Vue components
Prefer emit/@event-name for state changes over other communication patterns
Use defineExpose only for imperative operations (such as form.validate(), modal.open())
Replace PrimeVue Dropdown component with Select
Replace PrimeVue OverlayPanel component with Popover
Replace PrimeVue Calendar component with DatePicker
Replace PrimeVue InputSwitch component with ToggleSwitch
Replace PrimeVue Sidebar component with Drawer
Replace PrimeVue Chips component with AutoComplete with multiple enabled
Replace PrimeVue TabMenu component with Tabs without panels
Replace PrimeVue Steps component with Stepper without panels
Replace PrimeVue InlineMessage component with Message
Extract complex conditionals to computed properties
Implement cleanup for async operations in Vue components
Use lifecycle hooks: onMounted, onUpdated in Vue 3 Composition API
Use Teleport/Suspense when needed for component rendering
Define proper props and emits definitions in Vue componentsFiles:
src/components/queue/job/JobGroupsList.vuesrc/components/**/*.{vue,css}
📄 CodeRabbit inference engine (src/components/CLAUDE.md)
src/components/**/*.{vue,css}: Use Tailwind CSS only for styling (no custom CSS)
Use the correct tokens from style.css in the design system packageFiles:
src/components/queue/job/JobGroupsList.vuesrc/**/stores/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/stores/**/*.{ts,tsx}: Maintain clear public interfaces and restrict extension access in stores
Use TypeScript for type safety in state management storesFiles:
src/stores/queueStore.ts**/stores/*Store.ts
📄 CodeRabbit inference engine (AGENTS.md)
Name Pinia stores with the
*Store.tssuffixFiles:
src/stores/queueStore.ts🧠 Learnings (13)
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: src/lib/litegraph/CLAUDE.md:0-0 Timestamp: 2025-11-24T19:47:56.371Z Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setupApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: tests-ui/CLAUDE.md:0-0 Timestamp: 2025-11-24T19:48:03.270Z Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Write tests for new featuresApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: tests-ui/CLAUDE.md:0-0 Timestamp: 2025-11-24T19:48:03.270Z Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Use existing test utilities rather than writing custom utilitiesApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: tests-ui/CLAUDE.md:0-0 Timestamp: 2025-11-24T19:48:03.270Z Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Mock external dependencies in testsApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: .cursor/rules/unit-test.mdc:0-0 Timestamp: 2025-11-24T19:48:09.318Z Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Mocks should be cleanly written and easy to understand, with reusable mocks where possibleApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:48:03.270Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: tests-ui/CLAUDE.md:0-0 Timestamp: 2025-11-24T19:48:03.270Z Learning: Applies to tests-ui/**/*.test.{js,ts,jsx,tsx} : Always prefer vitest mock functions over writing verbose manual mocksApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: browser_tests/CLAUDE.md:0-0 Timestamp: 2025-11-24T19:47:22.909Z Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser testsApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: .cursor/rules/unit-test.mdc:0-0 Timestamp: 2025-11-24T19:48:09.318Z Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Use `test` instead of `it` for defining test cases in vitestApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:48:23.088Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-24T19:48:23.088Z Learning: Use Vitest (with happy-dom) for unit and component tests, and Playwright for E2E testsApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: .cursor/rules/unit-test.mdc:0-0 Timestamp: 2025-11-24T19:48:09.318Z Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Use `vitest` for unit testing in this projectApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: browser_tests/CLAUDE.md:0-0 Timestamp: 2025-11-24T19:47:22.909Z Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test across multiple viewportsApplied to files:
tests-ui/tests/composables/useResultGallery.test.ts📚 Learning: 2025-11-24T19:47:14.779Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-24T19:47:14.779Z Learning: Applies to **/*.{ts,tsx,vue} : Use `const settingStore = useSettingStore()` and `settingStore.get('Comfy.SomeSetting')` to retrieve settings in TypeScript/Vue filesApplied to files:
src/components/queue/job/JobDetailsPopover.stories.tssrc/stores/queueStore.ts📚 Learning: 2025-11-24T19:48:23.088Z
Learnt from: CR Repo: Comfy-Org/ComfyUI_frontend PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-24T19:48:23.088Z Learning: Source code should be organized in `src/` directory with key areas: `components/`, `views/`, `stores/` (Pinia), `composables/`, `services/`, `utils/`, `assets/`, `locales/`Applied to files:
src/stores/queueStore.ts🧬 Code graph analysis (3)
src/platform/remote/comfyui/jobs/types/jobTypes.ts (2)
src/schemas/apiSchema.ts (2)
resultItemType(13-13)zTaskOutput(154-154)src/composables/queue/useJobList.ts (1)
JobListItem(33-47)src/components/queue/job/JobDetailsPopover.stories.ts (2)
src/platform/remote/comfyui/jobs/types/jobTypes.ts (1)
JobListItem(115-115)src/stores/queueStore.ts (1)
TaskItemImpl(211-487)src/stores/queueStore.ts (5)
src/platform/remote/comfyui/jobs/types/jobTypes.ts (1)
JobListItem(115-115)src/composables/queue/useJobList.ts (1)
JobListItem(33-47)src/platform/remote/comfyui/jobs/index.ts (3)
JobListItem(14-14)fetchJobDetail(11-11)extractWorkflow(9-9)src/schemas/apiSchema.ts (1)
TaskOutput(155-155)src/platform/remote/comfyui/history/reconciliation.ts (1)
reconcileJobs(20-40)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: lint-and-format
- GitHub Check: test
- GitHub Check: collect
🔇 Additional comments (15)
src/components/queue/job/JobGroupsList.vue (1)
11-33: EnsureworkflowIdaccess path matches canonicalJobListItemshapeThe switch to
:workflow-id="ji.taskRef?.workflowId"looks aligned with the new Jobs API mapping (moving fromtaskRef.workflow.idtotaskRef.workflowId), but elsewhere in the PR the canonical field may be exposed directly onJobListItemasworkflowId. IfJobListItemnow has a top-levelworkflowId, it might be clearer and more robust to source it from there (optionally with a fallback totaskRef?.workflowIdonly if needed), so all queue/history consumers behave consistently.Please double‑check the
JobListItemtype and howworkflowIdis populated inuseJobList/ other queue components, and adjust this binding if the canonical field is actuallyji.workflowId.tests-ui/tests/composables/useResultGallery.test.ts (2)
8-23: Well-structured test helpers using the new JobListItem-based types.The
createResultItemhelper properly constructsResultItemImplinstances with overridden getters for test assertions. UsingObject.definePropertyto overrideurlandsupportsPreviewis a clean approach for test matching.
25-58: Test data factories align well with the new Jobs API types.
createMockJobandcreateTaskhelpers correctly constructJobListItemandTaskItemImplinstances matching the new API structure. ThecreateJobViewItemproperly typestaskRefto allowTaskItemImplreferences.src/platform/remote/comfyui/jobs/types/jobTypes.ts (3)
1-11: Well-documented module with clear Zod schema definitions.Good use of JSDoc for module documentation. Importing
resultItemTypeandzTaskOutputfrom the existing API schema maintains consistency with other type definitions.
112-116: Type derivation is clean and theJobListItemtype correctly enforces requiredpriority.The pattern of deriving types from Zod schemas ensures runtime and compile-time consistency. Making
priorityrequired inJobListItemwhile optional inRawJobListItemaligns with the PR's synthetic priority assignment strategy.
37-51: Thenode_idandnode_typefields should remain required.The schema correctly marks these fields as required. All test fixtures (useJobMenu.test.ts, useJobErrorReporting.test.ts) consistently include both fields, and the backend's
ExecutionErrorWsMessageschema in apiSchema.ts also marks them as required. No code patterns show conditional handling or fallbacks for missing node identification.src/components/queue/job/JobDetailsPopover.stories.ts (2)
55-65: Helper functions are well-structured for story data generation.The
makePendingTaskfunction properly delegates tomakeTaskwith appropriate status and timestamps. This pattern keeps the story setup code clean and consistent.
92-122:makeHistoryTaskcorrectly constructs execution timing and error data.The helper properly sets
execution_start_time,execution_end_time, and conditionally includesexecution_errorwith the expected structure fromzExecutionError. This aligns well with the new Jobs API types.src/stores/queueStore.ts (7)
211-233: Clean refactor ofTaskItemImplto useJobListItemas backing data.The constructor properly handles the case where no outputs are provided by synthesizing from
preview_output. Using_.mapValuesto stripanimatedoutputs maintains backward compatibility.
262-272: Clear mapping from job status to internal task type.The
taskTypegetter provides a clean abstraction over the job status values, defaulting non-active statuses to'History'. This handlescompleted,failed, andcancelledstatuses uniformly.
300-340: Good use of accessors to expose job data with appropriate fallbacks.The getters for
status,errorMessage,executionError,workflowId,workflow, andmessagesprovide clean access to job data. Returningundefinedforworkflowwith a comment directing users toloadWorkflow()is clear.
400-419: Error handling is properly delegated tofetchJobDetail.As confirmed in past reviews,
fetchJobDetailhas internal try-catch that returnsundefinedon failure. Theif (!jobDetail?.outputs)check correctly handles both error cases and missing data.
517-561: Well-structuredupdatefunction with proper reconciliation.The update function correctly:
- Fetches queue and history in parallel
- Maps API responses to
TaskItemImplinstances- Registers workflow ID mappings for execution tracking
- Uses
reconcileJobsfor history reconciliation- Reuses existing
TaskItemImplinstances to preserve identity
544-557: No action needed. The API return types are correctly defined insrc/scripts/api.ts:getQueue()returnsPromise<{ Running: JobListItem[], Pending: JobListItem[] }>andgetHistory()returnsPromise<JobListItem[]>. Both methods and the queueStore import the sameJobListItemtype from@/platform/remote/comfyui/jobs, and all usage in lines 526–527 and 545 correctly matches these type signatures. No type mismatches exist.
459-479: Theflatten()method creates synthetic IDs for display purposes only—these items appear in theflatTaskscomputed property but are not passed to API operations. ThedeleteTaskfunction uses the originaltask.promptIdfrom non-flattened items, so synthetic IDs never reach backend calls. The design appears intentional, with no evidence in the codebase that flattened items are used in delete or other API operations. However, without tracing all component usage offlatTasks, edge cases cannot be entirely ruled out.
| function makeTask( | ||
| id: string, | ||
| index: number, | ||
| createTimeMs?: number | ||
| priority: number, | ||
| overrides: Omit<Partial<JobListItem>, 'id' | 'priority'> & | ||
| Pick<JobListItem, 'status' | 'create_time' | 'update_time'> | ||
| ): TaskItemImpl { | ||
| const extraData = { | ||
| client_id: 'c1', | ||
| ...(typeof createTimeMs === 'number' ? { create_time: createTimeMs } : {}) | ||
| const job: JobListItem = { | ||
| id, | ||
| priority, | ||
| last_state_update: null, | ||
| ...overrides | ||
| } | ||
| return new TaskItemImpl('Pending', [index, id, {}, extraData, []]) | ||
| return new TaskItemImpl(job) | ||
| } |
There was a problem hiding this comment.
update_time and last_state_update are not defined in JobListItem schema.
The makeTask helper uses update_time (line 44) and last_state_update (line 49), but these fields are not defined in zRawJobListItem in jobTypes.ts. While .passthrough() allows extra fields at runtime, TypeScript won't recognize them, and the explicit type annotation on line 43-44 requiring update_time will cause type errors.
Either add these fields to the zRawJobListItem schema if they're part of the API, or remove them from the story helpers if they're not needed.
#!/bin/bash
# Verify if update_time or last_state_update are used elsewhere in the codebase
rg -n "update_time|last_state_update" --type=ts -g '!node_modules' | head -30🤖 Prompt for AI Agents
In src/components/queue/job/JobDetailsPopover.stories.ts around lines 40 to 53,
the makeTask helper sets update_time and last_state_update on the JobListItem
but those fields are not declared in the project's zRawJobListItem schema,
causing TypeScript type errors; either add update_time and last_state_update to
the zRawJobListItem/type definitions if these fields are returned by the API
(update the zod schema and corresponding TS types) or remove/stop using these
fields in the story helper and any callers so JobListItem only contains declared
properties; pick one approach and apply the change consistently across the story
helper and related tests/examples.
| export const zJobDetail = zRawJobListItem | ||
| .extend({ | ||
| workflow: z.unknown().optional(), | ||
| outputs: zTaskOutput.optional(), | ||
| update_time: z.number().optional(), | ||
| execution_status: z.unknown().optional(), | ||
| execution_meta: z.unknown().optional() | ||
| }) | ||
| .passthrough() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the z.unknown() fields or using stricter schemas.
The workflow, execution_status, and execution_meta fields use z.unknown(). While this provides flexibility, it loses type safety. If the structure is partially known, consider using a more specific schema with .passthrough() for the known parts, or add JSDoc comments explaining the expected shape.
Summary
Migrate frontend from legacy
/history,/history_v2, and/queueendpoints to the unified/jobsAPI with memory optimization and lazy loading.Changes
/history,/history_v2,/queue) with unified/jobsAPI/jobs/{id}endpointgetWorkflowFromHistorywrapper)reconcileHistorytoreconcileJobsfor clarityexecution_start_timeandexecution_end_timefrom API for execution timingworkflowIdfrom job instead of nestedworkflow.id/historyand/history_v2endpoints - requires backend Jobs API supportReview Focus
.passthrough()to allow extra API fields - ensures forward compatibility but less strict validationuseJobMenu:openJobWorkflowandexportJobWorkflownow fetch from API on demand instead of accessingtaskRef.workflowexecution_errorfield from API for rich error dialogs┆Issue is synchronized with this Notion page by Unito