feat(retry): ✨ Add model-switch retry for task/page flows#63
Conversation
AI Code Review SummaryPR: #63 (feat(retry): ✨ Add model-switch retry for task/page flows) Overall AssessmentNo blocking issue was detected in the reviewed diff; keep focused regression testing before merge. Major Findings by SeverityNo major issues identified from the reviewed diff. Actionable Suggestions
Potential Risks
Test Suggestions
File-Level Coverage Notes
Inline Downgraded Items (processed but not inline)
Coverage Status
Uncovered list:
No-patch covered list:
Runtime/Budget
Reasons:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const handleRetryTask = (id: string) => { | ||
| handleUpdateTaskStatus(id, 1, t('actions.retry')); | ||
| // 本地任务重试（支持切换模型） | ||
| const handleRetryTask = async (task: Task | CloudTask) => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| const { modelId, providerId } = parseModelValue(selectedModelValue); | ||
| const shouldOverride = selectedModelValue !== defaultModelValue; | ||
| const result = await window.api.task.retry({ | ||
| taskId: id, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -284,28 +314,60 @@ const Preview: React.FC = () => { | |||
|
|
|||
| // 重试任务（全部重试） | |||
| const handleRetryTask = async () => { | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| pages: 10, | ||
| completed_count: 5, | ||
| failed_count: 0, | ||
| provider: 1, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| }) | ||
| }) | ||
|
|
||
| it('retryTask/retryPage send model override payload when model is provided', async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| it('should update page status to COMPLETED', async () => { | ||
| let pageUpdateData: any; | ||
|
|
||
| vi.mocked(prisma.$transaction).mockImplementation(async (callback: any) => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| }) | ||
| }) | ||
|
|
||
| it('retryTask/retryPage send model override payload when model is provided', async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| expect(mockCloudService.deleteTask).toHaveBeenCalledWith('t1') | ||
| }) | ||
|
|
||
| it('passes model override for retryTask and retryPage', async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| }) | ||
| }) | ||
|
|
||
| it('should retry page with model override', async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ipcRenderer.invoke("taskDetail:getAllByTask", taskId), | ||
| retry: (pageId: number) => | ||
| ipcRenderer.invoke("taskDetail:retry", pageId), | ||
| retry: (params: number | { pageId: number; providerId?: number; modelId?: string }) => |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| provider: targetProvider, | ||
| model: targetModel, | ||
| model_name: targetModelName, | ||
| status: detailCount > 0 ? TaskStatus.PROCESSING : TaskStatus.PENDING, |
There was a problem hiding this comment.
[HIGH] Task retry sets status=PROCESSING without assigning workers, risking stuck tasks
On retry, tasks with existing page details are moved directly to PROCESSING while all worker assignments are cleared. If the orchestrator only picks up PENDING tasks/pages, this can leave retried tasks in PROCESSING but never re-queued.
Suggestion: Set retried tasks to PENDING consistently (and let scheduler transition to PROCESSING when actually claimed), or explicitly enqueue/emit an event path that guarantees PROCESSING tasks with reset pages are picked up immediately.
Risk: Retried tasks can appear active but make no progress, causing user-facing stalls and requiring manual intervention.
Confidence: 0.92
| }) | ||
| }) | ||
|
|
||
| describe('task:retry', () => { |
There was a problem hiding this comment.
[MEDIUM] Missing negative-path tests for model/provider validation in task retry handler
New tests do not cover key failure branches likely present in retry override logic (invalid provider/model combinations), leaving regressions undetected.
Suggestion: Add cases for: provider not found, provider inactive, model not found, model-provider mismatch, and DB errors during validation/update. Assert exact error payloads and that no event is emitted on failure.
Risk: Invalid override inputs could be accepted or fail unclearly in production without tests catching regressions.
Confidence: 0.90
| }) | ||
| }) | ||
|
|
||
| it('should retry page with model override', async () => { |
There was a problem hiding this comment.
[MEDIUM] Task detail retry tests lack assertions for status/progress/event side effects
The added test validates return shape but not orchestration side effects that are critical for worker scheduling and UI updates.
Suggestion: Assert tx.task.update invocation (expected status/progress transitions), taskDetail.update payload fields, and event bus emissions (or explicit non-emission) for retry operations.
Risk: Retry may return success while silently skipping task state/event updates, causing stuck queues or stale UI.
Confidence: 0.84
| IPC_CHANNELS.TASK.RETRY, | ||
| async ( | ||
| _, | ||
| params: string | { taskId: string; providerId?: number; modelId?: string } |
There was a problem hiding this comment.
[MEDIUM] Missing compatibility test for legacy task retry invocation shape
New dual input API (string or object) plus paired override validation adds branches that are easy to regress without explicit tests.
Suggestion: Add IPC handler tests for task:retry covering: (1) legacy string input success, (2) object input with no override success, (3) only providerId provided -> error, (4) only modelId provided -> error, (5) both provided with invalid provider/model -> error, (6) both valid -> provider/model switched on task and details.
Risk: Renderer/preload or older callers may silently break after refactors; partial override handling may regress and produce inconsistent retry behavior.
Confidence: 0.93
| }); | ||
|
|
||
| if (detailCount > 0) { | ||
| await tx.taskDetail.updateMany({ |
There was a problem hiding this comment.
[MEDIUM] No explicit tests for retry state-reset invariants and emitted events
Retry now performs a broad state reset (counts/tokens/content/provider/model) and emits two task events; these are critical invariants but not evidenced by tests in this diff.
Suggestion: Add transactional handler tests asserting post-retry invariants for both branches (detailCount > 0 and detailCount == 0), including status/progress counters reset, detail fields reset, provider/model propagation, and exact event emissions (TASK_UPDATED, TASK_STATUS_CHANGED) with expected payloads.
Risk: Without invariant/event tests, subtle regressions can cause stuck tasks, incorrect UI progress, or missing realtime updates despite successful API responses.
Confidence: 0.90
| }) | ||
| }) | ||
|
|
||
| it('retryTask/retryPage send model override payload when model is provided', async () => { |
There was a problem hiding this comment.
[LOW] CloudService override payload tests miss boundary case when model is omitted
Only the provided-model branch is tested; omission behavior (no body vs empty body) remains unguarded.
Suggestion: Add tests for retryTask(id) and retryPage(id,page) without model to assert exact request options and prevent accidental API contract drift.
Risk: A subtle request-shape regression could break backward compatibility with server endpoints expecting optional model fields.
Confidence: 0.85
| expect(window.api.cloud.getTaskPages).toHaveBeenCalledWith({ taskId: 't1', page: 1, pageSize: 2 }) | ||
| expect(window.api.cloud.cancelTask).toHaveBeenCalledWith('t1') | ||
| expect(window.api.cloud.retryTask).toHaveBeenCalledWith('t1') | ||
| expect(window.api.cloud.retryTask).toHaveBeenCalledWith({ id: 't1', model: 'pro' }) |
There was a problem hiding this comment.
[LOW] Multiple toHaveBeenCalledWith assertions on same mock can hide call-order/intent issues
The test now asserts two different argument shapes on the same mock with repeated toHaveBeenCalledWith, which only checks that each call happened at least once and may miss ordering or accidental extra calls.
Suggestion: Use toHaveBeenNthCalledWith (or inspect mock.calls) to verify exact sequence and payload per invocation. Optionally assert total call count to guard against unexpected extra calls.
Risk: False positives in tests can allow regressions in retry argument mapping/order to pass unnoticed.
Confidence: 0.88
| 'actions.cancel': 'Cancel', | ||
| 'actions.retry': 'Retry', | ||
| 'actions.delete': 'Delete', | ||
| 'retry.confirm_with_model': 'Retry Task (Choose Model)', |
There was a problem hiding this comment.
[LOW] List page mocks expanded for retry-model UX but no behavioral assertions added
Test setup now includes retry-with-model strings and methods, but no new assertions verify that UI uses them correctly.
Suggestion: Add tests for opening retry dialog, loading model list, handling load failure (retry.load_models_failed), empty models (retry.no_models_available), and submitting selected model.
Risk: Localization or retry dialog logic could regress with tests still passing due to unused mocks.
Confidence: 0.89
| expect(screen.queryByText('Retry All')).not.toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('should show retry all in More Actions menu for completed tasks', async () => { |
There was a problem hiding this comment.
[LOW] No assertion of retry-all action behavior after menu visibility in local Preview
The new test validates visibility of 'Retry All' but not that clicking it triggers task.retry with expected payload/model selection.
Suggestion: Extend test to click 'Retry All' and assert window.api.task.retry invocation shape, plus success/error feedback expectations.
Risk: Menu option can render correctly while being disconnected or incorrectly wired to backend action.
Confidence: 0.91
| label: t(`retry_model.${tier}`), | ||
| })); | ||
| const taskTier = (task?.model_tier || 'lite') as CloudModelTier; | ||
| let selectedModel: CloudModelTier = CLOUD_MODEL_TIERS.includes(taskTier) ? taskTier : 'lite'; |
There was a problem hiding this comment.
[LOW] Insufficient tests for cloud retry model defaulting/fallback logic
New fallback from task.model_tier to 'lite' lacks visible tests for invalid/unknown tiers and for preserving selected tier through modal interaction.
Suggestion: Add tests for: undefined model_tier, invalid model_tier, expected default selection, and API being called with user-selected tier after onChange.
Risk: Wrong model tier can be submitted silently, leading to unexpected cost/performance behavior for users.
Confidence: 0.87
1 participant
Summary
task:retry) with optional provider/model override, and extend page retry payloads for model switching.lite/pro/ultra) across service, IPC, preload, and context layers.More Actionsmenu.npm testtovitest runso default test command exits instead of watch mode.Test Plan
npm run test:unit -- --silentnpm run test:renderer -- --silentnpm run test:renderer -- --run src/renderer/pages/__tests__/CloudPreview.test.tsx src/renderer/pages/__tests__/Preview.test.tsx --silent🤖 Generated with Codex Cli