|
| 1 | +# Jobs API Migration Plan |
| 2 | + |
| 3 | +## Overview |
| 4 | + |
| 5 | +This document outlines the strategy for breaking up the Jobs API migration PR (#7125) into smaller, reviewable chunks. |
| 6 | + |
| 7 | +## Current State |
| 8 | + |
| 9 | +The PR migrates the frontend from legacy `/history` and `/queue` endpoints to the unified `/jobs` API. This involves: |
| 10 | + |
| 11 | +- **24 source files** changed |
| 12 | +- **16 test files** changed |
| 13 | +- Core changes to `TaskItemImpl` in `queueStore.ts` |
| 14 | + |
| 15 | +## Dependency Analysis |
| 16 | + |
| 17 | +``` |
| 18 | +jobTypes.ts (types) |
| 19 | + ↓ |
| 20 | +fetchJobs.ts (fetchers) |
| 21 | + ↓ |
| 22 | +api.ts (API layer) |
| 23 | + ↓ |
| 24 | +queueStore.ts (TaskItemImpl rewrite) |
| 25 | + ↓ |
| 26 | +┌───────────────────────────────────────┐ |
| 27 | +│ All consumers must update together: │ |
| 28 | +│ - useJobList.ts │ |
| 29 | +│ - useJobMenu.ts │ |
| 30 | +│ - useResultGallery.ts │ |
| 31 | +│ - useJobErrorReporting.ts │ |
| 32 | +│ - JobGroupsList.vue │ |
| 33 | +│ - assetsStore.ts │ |
| 34 | +│ - reconciliation.ts │ |
| 35 | +└───────────────────────────────────────┘ |
| 36 | +``` |
| 37 | + |
| 38 | +## Proposed PR Split |
| 39 | + |
| 40 | +### PR 1: Jobs API Infrastructure (Foundation) |
| 41 | +**Status**: Can merge independently |
| 42 | +**Risk**: Low - purely additive |
| 43 | + |
| 44 | +Files: |
| 45 | +``` |
| 46 | +src/platform/remote/comfyui/jobs/types/jobTypes.ts (new) |
| 47 | +src/platform/remote/comfyui/jobs/fetchers/fetchJobs.ts (new) |
| 48 | +src/platform/remote/comfyui/jobs/index.ts (new) |
| 49 | +src/scripts/api.ts (add new methods) |
| 50 | +``` |
| 51 | + |
| 52 | +Changes: |
| 53 | +- Add Zod schemas for Jobs API response types |
| 54 | +- Add `fetchQueue()` and `fetchHistory()` functions using `/jobs` endpoint |
| 55 | +- Add `getQueue()` and `getHistory()` methods to ComfyApi class |
| 56 | +- Export types from barrel file |
| 57 | + |
| 58 | +Tests: |
| 59 | +- Unit tests for fetchers |
| 60 | +- Integration tests for API methods |
| 61 | + |
| 62 | +**Why separate?** This is purely additive. The new code exists alongside the old code without breaking anything. Can be reviewed and merged first. |
| 63 | + |
| 64 | +--- |
| 65 | + |
| 66 | +### PR 2: Core Migration (TaskItemImpl + Consumers) |
| 67 | +**Status**: Requires PR 1 |
| 68 | +**Risk**: Medium - breaking changes to core data model |
| 69 | + |
| 70 | +Files: |
| 71 | +``` |
| 72 | +src/stores/queueStore.ts (rewrite TaskItemImpl) |
| 73 | +src/schemas/apiSchema.ts (type updates) |
| 74 | +src/composables/queue/useJobList.ts (use new TaskItemImpl) |
| 75 | +src/composables/queue/useJobMenu.ts (use new TaskItemImpl) |
| 76 | +src/composables/queue/useResultGallery.ts (use new TaskItemImpl) |
| 77 | +src/components/queue/job/useJobErrorReporting.ts (use new TaskItemImpl) |
| 78 | +src/components/queue/job/JobGroupsList.vue (fix workflowId access) |
| 79 | +src/components/queue/QueueProgressOverlay.vue (if needed) |
| 80 | +src/stores/assetsStore.ts (use JobListItem) |
| 81 | +src/platform/remote/comfyui/history/reconciliation.ts (work with JobListItem) |
| 82 | +src/platform/workflow/cloud/getWorkflowFromHistory.ts (use fetchJobDetail) |
| 83 | +src/scripts/ui.ts (type fix) |
| 84 | +``` |
| 85 | + |
| 86 | +Changes: |
| 87 | +- Rewrite `TaskItemImpl` to wrap `JobListItem` instead of legacy tuple format |
| 88 | +- Update all getters to derive from job properties |
| 89 | +- Update all consumers to use new property names |
| 90 | +- Update reconciliation to work with `JobListItem[]` |
| 91 | + |
| 92 | +Tests: |
| 93 | +- All queue-related tests |
| 94 | +- queueStore tests |
| 95 | +- Integration tests |
| 96 | + |
| 97 | +**Why together?** These changes are tightly coupled. `TaskItemImpl` API changes break all consumers, so they must be updated atomically. |
| 98 | + |
| 99 | +--- |
| 100 | + |
| 101 | +### PR 3: Cleanup Legacy Code |
| 102 | +**Status**: Requires PR 2 |
| 103 | +**Risk**: Low - removing unused code |
| 104 | + |
| 105 | +Files to DELETE: |
| 106 | +``` |
| 107 | +src/platform/remote/comfyui/history/adapters/v2ToV1Adapter.ts |
| 108 | +src/platform/remote/comfyui/history/fetchers/fetchHistoryV1.ts |
| 109 | +src/platform/remote/comfyui/history/fetchers/fetchHistoryV2.ts |
| 110 | +src/platform/remote/comfyui/history/types/historyV1Types.ts |
| 111 | +src/platform/remote/comfyui/history/types/historyV2Types.ts |
| 112 | +tests-ui/fixtures/historyFixtures.ts |
| 113 | +tests-ui/fixtures/historySortingFixtures.ts |
| 114 | ++ related test files |
| 115 | +``` |
| 116 | + |
| 117 | +Files to MODIFY: |
| 118 | +``` |
| 119 | +src/platform/remote/comfyui/history/index.ts (remove old exports) |
| 120 | +src/platform/remote/comfyui/history/types/index.ts (remove old exports) |
| 121 | +``` |
| 122 | + |
| 123 | +**Why separate?** Deletion is low-risk but should be done after confirming the new code works in production. Allows rollback if issues are found. |
| 124 | + |
| 125 | +--- |
| 126 | + |
| 127 | +## Alternative: Feature Flag Approach |
| 128 | + |
| 129 | +If the above split is still too risky, consider: |
| 130 | + |
| 131 | +1. Add feature flag `useJobsApi` (default: false) |
| 132 | +2. Keep both code paths in TaskItemImpl |
| 133 | +3. Gradually roll out via feature flag |
| 134 | +4. Remove old code path after validation |
| 135 | + |
| 136 | +This is more complex but allows incremental rollout. |
| 137 | + |
| 138 | +--- |
| 139 | + |
| 140 | +## Recommended Order |
| 141 | + |
| 142 | +1. **PR 1** → Merge first (no risk) |
| 143 | +2. **PR 2** → Merge after PR 1 (main migration) |
| 144 | +3. **PR 3** → Merge after validating PR 2 in production |
| 145 | + |
| 146 | +## Current PR Status |
| 147 | + |
| 148 | +The current PR (#7125) contains PR 1 + PR 2 combined. To split: |
| 149 | + |
| 150 | +1. Create new branch from main |
| 151 | +2. Cherry-pick only the Jobs API infrastructure commits |
| 152 | +3. Open PR 1 |
| 153 | +4. Rebase current branch on PR 1 after merge |
| 154 | +5. Current branch becomes PR 2 |
| 155 | + |
| 156 | +--- |
| 157 | + |
| 158 | +## Files by PR |
| 159 | + |
| 160 | +### PR 1 Files (8 files) |
| 161 | +``` |
| 162 | +src/platform/remote/comfyui/jobs/types/jobTypes.ts |
| 163 | +src/platform/remote/comfyui/jobs/fetchers/fetchJobs.ts |
| 164 | +src/platform/remote/comfyui/jobs/index.ts |
| 165 | +src/scripts/api.ts |
| 166 | ++ 4 test files |
| 167 | +``` |
| 168 | + |
| 169 | +### PR 2 Files (~28 files) |
| 170 | +``` |
| 171 | +src/stores/queueStore.ts |
| 172 | +src/stores/assetsStore.ts |
| 173 | +src/schemas/apiSchema.ts |
| 174 | +src/scripts/ui.ts |
| 175 | +src/composables/queue/useJobList.ts |
| 176 | +src/composables/queue/useJobMenu.ts |
| 177 | +src/composables/queue/useResultGallery.ts |
| 178 | +src/components/queue/job/useJobErrorReporting.ts |
| 179 | +src/components/queue/job/JobGroupsList.vue |
| 180 | +src/components/queue/job/JobDetailsPopover.stories.ts |
| 181 | +src/components/queue/QueueProgressOverlay.vue |
| 182 | +src/platform/remote/comfyui/history/reconciliation.ts |
| 183 | +src/platform/remote/comfyui/history/index.ts |
| 184 | +src/platform/workflow/cloud/getWorkflowFromHistory.ts |
| 185 | +src/platform/workflow/cloud/index.ts |
| 186 | +browser_tests/fixtures/ComfyPage.ts |
| 187 | +browser_tests/fixtures/utils/taskHistory.ts |
| 188 | ++ ~12 test files |
| 189 | +``` |
| 190 | + |
| 191 | +### PR 3 Files (~12 files to delete) |
| 192 | +``` |
| 193 | +DELETE: src/platform/remote/comfyui/history/adapters/* |
| 194 | +DELETE: src/platform/remote/comfyui/history/fetchers/fetchHistoryV1.ts |
| 195 | +DELETE: src/platform/remote/comfyui/history/fetchers/fetchHistoryV2.ts |
| 196 | +DELETE: src/platform/remote/comfyui/history/types/historyV1Types.ts |
| 197 | +DELETE: src/platform/remote/comfyui/history/types/historyV2Types.ts |
| 198 | +DELETE: tests-ui/fixtures/history*.ts |
| 199 | +DELETE: related test files |
| 200 | +MODIFY: index.ts files to remove exports |
| 201 | +``` |
0 commit comments