Conversation
📝 WalkthroughWalkthroughThis PR refactors delegated-task eviction, preserves interrupted parent-child links, adds explicit subtask abandonment through the API and webview, updates delegated/interrupted task UI, and adds unit and end-to-end coverage for interruption, abandonment, redelegation, and resumed child completion. ChangesSubtask lifecycle and abandonment
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
edelauna
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
webview-ui/src/i18n/locales/ca/chat.json (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTranslate new localization keys.
The new localization keys for subtask states were added with English fallback strings across non-English locales. Updating them to their respective translated values ensures a consistent localized user experience.
webview-ui/src/i18n/locales/ca/chat.json#L21-L22: Update to Catalan:"waitingOnSubtask": "Esperant la subtasca","goToSubtask": "Anar a la subtasca".webview-ui/src/i18n/locales/ca/history.json#L58-L59: Update to Catalan:"delegatedTag": "Esperant la subtasca","interruptedTag": "Interromput".webview-ui/src/i18n/locales/de/chat.json#L21-L22: Update to German:"waitingOnSubtask": "Warten auf Teilaufgabe","goToSubtask": "Zur Teilaufgabe".webview-ui/src/i18n/locales/de/history.json#L58-L59: Update to German:"delegatedTag": "Warten auf Teilaufgabe","interruptedTag": "Unterbrochen".webview-ui/src/i18n/locales/es/chat.json#L21-L22: Update to Spanish:"waitingOnSubtask": "Esperando subtarea","goToSubtask": "Ir a la subtarea".webview-ui/src/i18n/locales/es/history.json#L58-L59: Update to Spanish:"delegatedTag": "Esperando subtarea","interruptedTag": "Interrumpido".webview-ui/src/i18n/locales/fr/chat.json#L21-L22: Update to French:"waitingOnSubtask": "En attente de la sous-tâche","goToSubtask": "Aller à la sous-tâche".webview-ui/src/i18n/locales/fr/history.json#L58-L59: Update to French:"delegatedTag": "En attente de la sous-tâche","interruptedTag": "Interrompu".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode-e2e/src/fixtures/subtasks.ts`:
- Around line 388-392: Update the child fixture’s predicate to exclude requests
containing SUBTASK_ABANDON_PARENT_PROMPT, matching the interrupted scenario’s
parent-request exclusion. Keep the existing child marker, tool-call, and
follow-up answer checks unchanged so only the intended child request is
accepted.
In `@src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts`:
- Around line 809-810: Replace the fixed 10 ms sleeps after TaskCompleted
emissions in the task-history tests with awaited completion of the listener
promises. Update the fake emitter used by ClineProvider task-history tests to
collect listener return promises and make emit() await them, then await emit()
directly at the affected cases around task-cb-1 and the additional TaskCompleted
scenarios.
In `@src/core/webview/ClineProvider.ts`:
- Around line 533-540: Update provider disposal to call evictCurrentTask()
instead of removeClineFromStack(), ensuring an active delegated child is marked
interrupted during shutdown while preserving the existing disposal flow.
- Around line 600-609: The in-lock transition in markDelegatedChildInterrupted
must proceed only when childHistory.status is exactly "active". Update the guard
to skip every other status, including "completed", then validate the
active-to-interrupted transition before calling
updateTaskHistory(interruptedChild).
- Around line 3599-3615: Update the delegation flow around atomicReadAndUpdate
to execute under runDelegationTransition(), removing the pre-lock
existingAwaitedChildStatus snapshot. Inside the updater, read
historyItem.awaitingChildId and its current status together, and only sever the
link when that exact child is confirmed interrupted; otherwise throw and
preserve rollback behavior.
- Around line 541-553: Ensure interruption-persistence failures are propagated
instead of swallowed: update markDelegatedChildInterrupted and its callers,
including evictCurrentTask and the corresponding paths near createTask and
abandonSubtask, to preserve rejection. Remove ineffective non-fatal catches and
prevent createTask from opening a replacement task when eviction fails, leaving
the live child/history state consistent.
In `@webview-ui/src/i18n/locales/nl/chat.json`:
- Around line 21-22: Replace the English fallback values for waitingOnSubtask
and goToSubtask with accurate Dutch translations in
webview-ui/src/i18n/locales/nl/chat.json:21-22, Polish translations in
webview-ui/src/i18n/locales/pl/chat.json:21-22, Brazilian Portuguese
translations in webview-ui/src/i18n/locales/pt-BR/chat.json:21-22, Russian
translations in webview-ui/src/i18n/locales/ru/chat.json:21-22, and Turkish
translations in webview-ui/src/i18n/locales/tr/chat.json:21-22. Also localize
the newly added status-label keys in
webview-ui/src/i18n/locales/nl/history.json:51-52,
webview-ui/src/i18n/locales/pl/history.json:51-52,
webview-ui/src/i18n/locales/pt-BR/history.json:51-52, and
webview-ui/src/i18n/locales/ru/history.json:51-52, preserving the existing keys
and JSON structure.
In `@webview-ui/src/i18n/locales/tr/history.json`:
- Around line 51-52: Replace the English placeholder values for delegatedTag and
interruptedTag with Turkish translations in
webview-ui/src/i18n/locales/tr/history.json:51-52. Add Vietnamese navigation
labels in webview-ui/src/i18n/locales/vi/chat.json:21-22 and Vietnamese status
labels in webview-ui/src/i18n/locales/vi/history.json:51-52. Add Simplified
Chinese navigation and status labels in
webview-ui/src/i18n/locales/zh-CN/chat.json:21-22 and
webview-ui/src/i18n/locales/zh-CN/history.json:51-52, and Traditional Chinese
equivalents in webview-ui/src/i18n/locales/zh-TW/chat.json:21-22 and
webview-ui/src/i18n/locales/zh-TW/history.json:51-52; preserve the existing
translation keys and JSON structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 285a6a43-99ec-47fe-afa2-80fc1187e121
📒 Files selected for processing (61)
apps/vscode-e2e/src/fixtures/subtasks.tsapps/vscode-e2e/src/suite/subtasks.test.tspackages/types/src/api.tspackages/types/src/vscode-extension-host.tssrc/__tests__/abandonSubtask.spec.tssrc/__tests__/api-subtask.spec.tssrc/__tests__/helpers/provider-stub.tssrc/__tests__/history-resume-delegation.spec.tssrc/__tests__/provider-delegation.spec.tssrc/__tests__/removeClineFromStack-delegation.spec.tssrc/__tests__/single-open-invariant.spec.tssrc/activate/__tests__/registerCommands.spec.tssrc/activate/registerCommands.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.tssrc/core/webview/__tests__/ClineProvider.taskHistory.spec.tssrc/core/webview/__tests__/webviewMessageHandler.abandonSubtask.spec.tssrc/core/webview/webviewMessageHandler.tssrc/extension/api.tswebview-ui/src/components/chat/TaskHeader.tsxwebview-ui/src/components/chat/__tests__/TaskHeader.spec.tsxwebview-ui/src/components/history/SubtaskRow.tsxwebview-ui/src/components/history/TaskItemFooter.tsxwebview-ui/src/components/history/TaskStatusBadge.tsxwebview-ui/src/components/history/__tests__/TaskItemFooter.spec.tsxwebview-ui/src/i18n/locales/ca/chat.jsonwebview-ui/src/i18n/locales/ca/history.jsonwebview-ui/src/i18n/locales/de/chat.jsonwebview-ui/src/i18n/locales/de/history.jsonwebview-ui/src/i18n/locales/en/chat.jsonwebview-ui/src/i18n/locales/en/history.jsonwebview-ui/src/i18n/locales/es/chat.jsonwebview-ui/src/i18n/locales/es/history.jsonwebview-ui/src/i18n/locales/fr/chat.jsonwebview-ui/src/i18n/locales/fr/history.jsonwebview-ui/src/i18n/locales/hi/chat.jsonwebview-ui/src/i18n/locales/hi/history.jsonwebview-ui/src/i18n/locales/id/chat.jsonwebview-ui/src/i18n/locales/id/history.jsonwebview-ui/src/i18n/locales/it/chat.jsonwebview-ui/src/i18n/locales/it/history.jsonwebview-ui/src/i18n/locales/ja/chat.jsonwebview-ui/src/i18n/locales/ja/history.jsonwebview-ui/src/i18n/locales/ko/chat.jsonwebview-ui/src/i18n/locales/ko/history.jsonwebview-ui/src/i18n/locales/nl/chat.jsonwebview-ui/src/i18n/locales/nl/history.jsonwebview-ui/src/i18n/locales/pl/chat.jsonwebview-ui/src/i18n/locales/pl/history.jsonwebview-ui/src/i18n/locales/pt-BR/chat.jsonwebview-ui/src/i18n/locales/pt-BR/history.jsonwebview-ui/src/i18n/locales/ru/chat.jsonwebview-ui/src/i18n/locales/ru/history.jsonwebview-ui/src/i18n/locales/tr/chat.jsonwebview-ui/src/i18n/locales/tr/history.jsonwebview-ui/src/i18n/locales/vi/chat.jsonwebview-ui/src/i18n/locales/vi/history.jsonwebview-ui/src/i18n/locales/zh-CN/chat.jsonwebview-ui/src/i18n/locales/zh-CN/history.jsonwebview-ui/src/i18n/locales/zh-TW/chat.jsonwebview-ui/src/i18n/locales/zh-TW/history.json
| predicate: (req: ChatCompletionRequest) => | ||
| requestContains(req, [SUBTASK_ABANDON_CHILD_MARKER]) && | ||
| !requestContains(req, ["call_abandon_child_followup_001"]) && | ||
| !requestContains(req, [`<user_message>\\n${SUBTASK_ABANDON_CHILD_FOLLOWUP_ANSWER}\\n</user_message>`]), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude the embedded parent marker from the child fixture.
SUBTASK_ABANDON_PARENT_PROMPT contains SUBTASK_ABANDON_CHILD_MARKER, so this predicate also accepts parent requests. A later parent turn can therefore receive the child's follow-up response. Mirror the interrupted scenario’s exclusion.
Proposed fix
predicate: (req: ChatCompletionRequest) =>
requestContains(req, [SUBTASK_ABANDON_CHILD_MARKER]) &&
+ !requestContains(req, [SUBTASK_ABANDON_PARENT_MARKER]) &&
!requestContains(req, ["call_abandon_child_followup_001"]) &&📝 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.
| predicate: (req: ChatCompletionRequest) => | |
| requestContains(req, [SUBTASK_ABANDON_CHILD_MARKER]) && | |
| !requestContains(req, ["call_abandon_child_followup_001"]) && | |
| !requestContains(req, [`<user_message>\\n${SUBTASK_ABANDON_CHILD_FOLLOWUP_ANSWER}\\n</user_message>`]), | |
| }, | |
| predicate: (req: ChatCompletionRequest) => | |
| requestContains(req, [SUBTASK_ABANDON_CHILD_MARKER]) && | |
| !requestContains(req, [SUBTASK_ABANDON_PARENT_MARKER]) && | |
| !requestContains(req, ["call_abandon_child_followup_001"]) && | |
| !requestContains(req, [`<user_message>\\n${SUBTASK_ABANDON_CHILD_FOLLOWUP_ANSWER}\\n</user_message>`]), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/vscode-e2e/src/fixtures/subtasks.ts` around lines 388 - 392, Update the
child fixture’s predicate to exclude requests containing
SUBTASK_ABANDON_PARENT_PROMPT, matching the interrupted scenario’s
parent-request exclusion. Keep the existing child marker, tool-call, and
follow-up answer checks unchanged so only the intended child request is
accepted.
There was a problem hiding this comment.
Confirmed at c13507c. SUBTASK_ABANDON_PARENT_PROMPT embeds SUBTASK_ABANDON_CHILD_PROMPT verbatim (fixtures:31-35), so any later parent turn satisfies the bare child-marker substring check — exactly the pitfall the interrupted scenario documents and excludes at fixtures:296-299. Latent today since the abandon e2e never sends the parent back to the mock after the abandon, but the fixture should mirror the interrupted one and exclude SUBTASK_ABANDON_PARENT_MARKER before it bites a future test.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| fakeTask.emit(RooCodeEventName.TaskCompleted, "task-cb-1", {}, {}) | ||
| await new Promise((r) => setTimeout(r, 10)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Await the completion listener instead of sleeping for 10 ms.
These fixed delays can expire before the asynchronous history write or rejection handler finishes. Have the fake emitter collect and await listener promises, then await emit() directly.
Also applies to: 825-826, 848-849
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts` around lines
809 - 810, Replace the fixed 10 ms sleeps after TaskCompleted emissions in the
task-history tests with awaited completion of the listener promises. Update the
fake emitter used by ClineProvider task-history tests to collect listener return
promises and make emit() await them, then await emit() directly at the affected
cases around task-cb-1 and the additional TaskCompleted scenarios.
There was a problem hiding this comment.
Confirmed at c13507c. The fake task's emit discards listener return values, so the 10 ms setTimeout is the only thing standing between the async onTaskCompleted write and the assertion (also 825-826, 848-849). Collecting listener promises in emit and awaiting it directly would make these deterministic.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| /** | ||
| * Evicts the current task from the stack and, if it was an active delegated child, | ||
| * marks it interrupted so the parent stays delegated (rather than silently losing the link). | ||
| * | ||
| * Use this in place of bare removeClineFromStack() at any call site that is not itself | ||
| * part of a delegation transition (i.e. everywhere except delegateParentAndOpenChild, | ||
| * createTask with a parentTask, and reopenParentFromDelegation). | ||
| */ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Route provider disposal through evictCurrentTask().
Line 685 still calls bare removeClineFromStack() while disposing the provider. An active delegated child therefore remains persisted as active across extension shutdown/reload instead of becoming interrupted, contradicting this method’s stated call-site contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/ClineProvider.ts` around lines 533 - 540, Update provider
disposal to call evictCurrentTask() instead of removeClineFromStack(), ensuring
an active delegated child is marked interrupted during shutdown while preserving
the existing disposal flow.
There was a problem hiding this comment.
Confirmed at c13507c — dispose() (ClineProvider.ts:684-687) still loops on bare removeClineFromStack(), so an active delegated child at shutdown stays persisted as active with nothing running it after reload, and the parent stays delegated with no interrupted badge and no abandon path (which requires interrupted). The other non-delegation call sites this PR left alone (deleteTaskWithId, resetState, webview-init cleanup) have reasonable excuses, but dispose() matches the stated evictCurrentTask() contract ("any call site that is not itself part of a delegation transition"). Worth routing through evictCurrentTask() — the extra webview posts during dispose are harmless since postMessageToWebview is a no-op once the view is gone.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| public async evictCurrentTask(): Promise<void> { | ||
| const current = this.getCurrentTask() | ||
| const storedHistory = current ? this.taskHistoryStore.get(current.taskId) : undefined | ||
| await this.removeClineFromStack() | ||
| if (storedHistory?.status === "active" && storedHistory.parentTaskId) { | ||
| await this.markDelegatedChildInterrupted({ | ||
| childTaskId: storedHistory.id, | ||
| parentTaskId: storedHistory.parentTaskId, | ||
| }).catch((err) => | ||
| this.log( | ||
| `[evictCurrentTask] markDelegatedChildInterrupted failed (non-fatal): ${err instanceof Error ? err.message : String(err)}`, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not report successful eviction when interruption persistence fails.
markDelegatedChildInterrupted() swallows its own error, making the caller’s .catch() ineffective; createTask() then suppresses eviction failures too. The live child is removed while history can remain active with its parent still delegated, and abandonSubtask() subsequently rejects it. Propagate the failure or atomically fail closed before opening the replacement task.
Also applies to: 616-619, 3140-3144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/ClineProvider.ts` around lines 541 - 553, Ensure
interruption-persistence failures are propagated instead of swallowed: update
markDelegatedChildInterrupted and its callers, including evictCurrentTask and
the corresponding paths near createTask and abandonSubtask, to preserve
rejection. Remove ineffective non-fatal catches and prevent createTask from
opening a replacement task when eviction fails, leaving the live child/history
state consistent.
There was a problem hiding this comment.
Confirmed against the current head (c13507c). markDelegatedChildInterrupted catches everything internally (ClineProvider.ts:615-619), so the .catch() in evictCurrentTask (551) is unreachable and createTask's evictCurrentTask().catch(() => {}) (3142) can't observe a persistence failure either. Net effect when the interrupted write fails: the live child is already popped, history still says active, the parent stays delegated, and — since abandonSubtask requires interrupted — the stuck child can't be abandoned from the API either. Rethrowing (or returning a success flag the caller can act on) would make the failure visible; at minimum the dead .catch should go so the contract is honest.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| // Re-check inside the lock to close the TOCTOU window with cancelTask(). | ||
| if (childHistory?.status === "interrupted") { | ||
| this.log( | ||
| `[markDelegatedChildInterrupted] Child ${childTaskId} already interrupted (in-lock check) — skipping`, | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| const interruptedChild = { ...childHistory, status: "interrupted" as const } | ||
| await this.updateTaskHistory(interruptedChild) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only transition a freshly confirmed active child to interrupted.
The in-lock check skips only interrupted. If completion wins the race after evictCurrentTask() snapshots active, this code overwrites the fresh completed status with interrupted. Skip unless childHistory.status === "active" and validate the transition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/ClineProvider.ts` around lines 600 - 609, The in-lock
transition in markDelegatedChildInterrupted must proceed only when
childHistory.status is exactly "active". Update the guard to skip every other
status, including "completed", then validate the active-to-interrupted
transition before calling updateTaskHistory(interruptedChild).
There was a problem hiding this comment.
Pushback on this one after checking the write path at c13507c: updateTaskHistory → TaskHistoryStore.upsertCore enforces assertValidTransition at the write boundary (TaskHistoryStore.ts:207-212), and completed → interrupted is not in VALID_TRANSITIONS. So if completion wins the race, the interrupted write throws, lands in the existing catch, and the completed status survives — the outcome is a noisy log line, not a stomped status. Skipping non-active statuses in the lock would still be a nice clarity/robustness tweak (and would silence the spurious error log), but this isn't the data-integrity bug it reads as.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| // We snapshot the old awaited child's status BEFORE entering the updater (which is | ||
| // synchronous) so the guard can verify the child is actually interrupted before | ||
| // severing. An active child must never be silently detached. | ||
| const existingParent = this.taskHistoryStore.get(parentTaskId) | ||
| const existingAwaitedChildStatus = existingParent?.awaitingChildId | ||
| ? this.taskHistoryStore.get(existingParent.awaitingChildId)?.status | ||
| : undefined | ||
| try { | ||
| await this.taskHistoryStore.atomicReadAndUpdate(parentTaskId, (historyItem) => { | ||
| assertValidTransition(historyItem.status, "delegated") | ||
| const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId])) | ||
| let base = historyItem | ||
| if (historyItem.status === "delegated") { | ||
| // Only sever the stale link when the old child is confirmed interrupted. | ||
| // If it is still active, throw so the rollback path cleans up the new child | ||
| // rather than silently detaching a live task. | ||
| if (existingAwaitedChildStatus !== "interrupted") { | ||
| throw new Error( | ||
| `[delegateParentAndOpenChild] Cannot re-delegate: existing child ${historyItem.awaitingChildId} is ${existingAwaitedChildStatus}, not interrupted`, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Revalidate the exact awaited child under the delegation lock.
existingAwaitedChildStatus is captured before the atomic parent update and is not tied to the updater’s current historyItem.awaitingChildId. A concurrent delegation can replace the awaited child with an active one, after which the stale "interrupted" snapshot authorizes severing that active link. Serialize this section with runDelegationTransition() and revalidate the current child ID/status together.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/webview/ClineProvider.ts` around lines 3599 - 3615, Update the
delegation flow around atomicReadAndUpdate to execute under
runDelegationTransition(), removing the pre-lock existingAwaitedChildStatus
snapshot. Inside the updater, read historyItem.awaitingChildId and its current
status together, and only sever the link when that exact child is confirmed
interrupted; otherwise throw and preserve rollback behavior.
There was a problem hiding this comment.
Confirmed at c13507c. The updater reads the parent's current awaitingChildId but validates it against existingAwaitedChildStatus, which was snapshotted before atomicReadAndUpdate (ClineProvider.ts:3595-3598). If the awaited child changed between the snapshot and the updater running, the guard authorizes severing based on the wrong child. The fix is cheaper than the comment suggests — the updater is synchronous and taskHistoryStore.get() is in-memory, so you can read historyItem.awaitingChildId and that child's current status together inside the updater and throw when it isn't interrupted. Note the store's transition check does not backstop this path, since delegated → active is a valid transition for the parent.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| "waitingOnSubtask": "Waiting on subtask", | ||
| "goToSubtask": "Go to subtask" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize all newly added strings.
The same English fallback text was added to multiple non-English locale files, so users see untranslated navigation and status labels.
webview-ui/src/i18n/locales/nl/chat.json#L21-L22: provide Dutch task-navigation translations.webview-ui/src/i18n/locales/nl/history.json#L51-L52: provide Dutch status-label translations.webview-ui/src/i18n/locales/pl/chat.json#L21-L22: provide Polish task-navigation translations.webview-ui/src/i18n/locales/pl/history.json#L51-L52: provide Polish status-label translations.webview-ui/src/i18n/locales/pt-BR/chat.json#L21-L22: provide Brazilian Portuguese task-navigation translations.webview-ui/src/i18n/locales/pt-BR/history.json#L51-L52: provide Brazilian Portuguese status-label translations.webview-ui/src/i18n/locales/ru/chat.json#L21-L22: provide Russian task-navigation translations.webview-ui/src/i18n/locales/ru/history.json#L51-L52: provide Russian status-label translations.webview-ui/src/i18n/locales/tr/chat.json#L21-L22: provide Turkish task-navigation translations.
📍 Affects 9 files
webview-ui/src/i18n/locales/nl/chat.json#L21-L22(this comment)webview-ui/src/i18n/locales/nl/history.json#L51-L52webview-ui/src/i18n/locales/pl/chat.json#L21-L22webview-ui/src/i18n/locales/pl/history.json#L51-L52webview-ui/src/i18n/locales/pt-BR/chat.json#L21-L22webview-ui/src/i18n/locales/pt-BR/history.json#L51-L52webview-ui/src/i18n/locales/ru/chat.json#L21-L22webview-ui/src/i18n/locales/ru/history.json#L51-L52webview-ui/src/i18n/locales/tr/chat.json#L21-L22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/i18n/locales/nl/chat.json` around lines 21 - 22, Replace the
English fallback values for waitingOnSubtask and goToSubtask with accurate Dutch
translations in webview-ui/src/i18n/locales/nl/chat.json:21-22, Polish
translations in webview-ui/src/i18n/locales/pl/chat.json:21-22, Brazilian
Portuguese translations in webview-ui/src/i18n/locales/pt-BR/chat.json:21-22,
Russian translations in webview-ui/src/i18n/locales/ru/chat.json:21-22, and
Turkish translations in webview-ui/src/i18n/locales/tr/chat.json:21-22. Also
localize the newly added status-label keys in
webview-ui/src/i18n/locales/nl/history.json:51-52,
webview-ui/src/i18n/locales/pl/history.json:51-52,
webview-ui/src/i18n/locales/pt-BR/history.json:51-52, and
webview-ui/src/i18n/locales/ru/history.json:51-52, preserving the existing keys
and JSON structure.
There was a problem hiding this comment.
Verified at c13507c: the new waitingOnSubtask/goToSubtask and delegatedTag/interruptedTag values are English in all 16 non-English locales (checked de, fr, nl directly). These dictionaries are otherwise fully translated (e.g. backToParentTask is localized in de/nl), so the placeholders read as mixed-language UI rather than an established fallback convention here. If translation is intentionally follow-up work, worth saying so in the PR description, which currently claims the keys are "present in all 18 locale files".
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| "delegatedTag": "Waiting on subtask", | ||
| "interruptedTag": "Interrupted" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Replace the English placeholders in non-English locale dictionaries.
The new navigation and status strings are rendered directly, so these locales currently show mixed-language UI.
webview-ui/src/i18n/locales/tr/history.json#L51-L52: provide Turkish status labels.webview-ui/src/i18n/locales/vi/chat.json#L21-L22: provide Vietnamese navigation labels.webview-ui/src/i18n/locales/vi/history.json#L51-L52: provide Vietnamese status labels.webview-ui/src/i18n/locales/zh-CN/chat.json#L21-L22: provide Simplified Chinese navigation labels.webview-ui/src/i18n/locales/zh-CN/history.json#L51-L52: provide Simplified Chinese status labels.webview-ui/src/i18n/locales/zh-TW/chat.json#L21-L22: provide Traditional Chinese navigation labels.webview-ui/src/i18n/locales/zh-TW/history.json#L51-L52: provide Traditional Chinese status labels.
📍 Affects 7 files
webview-ui/src/i18n/locales/tr/history.json#L51-L52(this comment)webview-ui/src/i18n/locales/vi/chat.json#L21-L22webview-ui/src/i18n/locales/vi/history.json#L51-L52webview-ui/src/i18n/locales/zh-CN/chat.json#L21-L22webview-ui/src/i18n/locales/zh-CN/history.json#L51-L52webview-ui/src/i18n/locales/zh-TW/chat.json#L21-L22webview-ui/src/i18n/locales/zh-TW/history.json#L51-L52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/i18n/locales/tr/history.json` around lines 51 - 52, Replace
the English placeholder values for delegatedTag and interruptedTag with Turkish
translations in webview-ui/src/i18n/locales/tr/history.json:51-52. Add
Vietnamese navigation labels in webview-ui/src/i18n/locales/vi/chat.json:21-22
and Vietnamese status labels in
webview-ui/src/i18n/locales/vi/history.json:51-52. Add Simplified Chinese
navigation and status labels in
webview-ui/src/i18n/locales/zh-CN/chat.json:21-22 and
webview-ui/src/i18n/locales/zh-CN/history.json:51-52, and Traditional Chinese
equivalents in webview-ui/src/i18n/locales/zh-TW/chat.json:21-22 and
webview-ui/src/i18n/locales/zh-TW/history.json:51-52; preserve the existing
translation keys and JSON structure.
There was a problem hiding this comment.
Same verification on this set at c13507c: tr, vi, zh-CN, and zh-TW also carry the English values for both new key pairs. Consolidated into the note on the nl/chat thread — one fix pass across all 16 non-English locale files covers both.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
github-actions
Bot
added
the
awaiting-review
|
7 issues outstanding. Findings verified against the code were posted as replies on the matching review threads; the one finding with no existing thread is carried below.
Reviewed c13507c |
1 participant
Related GitHub Issue
Closes: #559
Description
This PR implements the Abandon Subtask flow end-to-end: a parent task that is waiting on a delegated subtask can sever the link if that subtask is interrupted, restoring the parent to active without losing the subtask's history.
Key changes:
Task lifecycle (
ClineProvider,webviewMessageHandler,extension/api):abandonSubtask()atomically severs the parent-child link in the history store, transitions parentdelegated → active, and closes the live child instance before the write so a latersaveClineMessagescan't silently reattach it. A TOCTOU guard re-checks child status inside the per-parent delegation lock.evictCurrentTask(): public method that snapshots the current task, pops it viaremoveClineFromStack, then callsmarkDelegatedChildInterruptedif the child was active+delegated — so the+button andclearCurrentTaskboth correctly interrupt an active delegated child rather than losing the delegation state.onTaskCompletedclosure: explicitly transitions the task tocompletedin the history store before re-emitting the event, ensuring a priorinterruptedstatus (from a cancel followed by resumption and completion) is overwritten rather than preserved by the store's merge.History UI (
HistoryView,TaskGroupItem,TaskItem,TaskItemFooter,SubtaskRow): delegation status badges (delegatedTag,interruptedTag) and an Abandon Subtask button gated onchild.status === "interrupted". The button opens a confirmation dialog before callingabandonSubtaskWithId.i18n: all new keys present in all 18 locale files (
en+ 17 others).E2E: dedicated fixtures and markers for the abandon scenario (separate from the interrupt scenario) to avoid
sequenceIndexexhaustion across sibling tests.Test coverage additions:
evictCurrentTask,markDelegatedChildInterrupted(TOCTOU + error paths),onTaskCompleted(real closure viataskCreationCallback)abandonSubtask: sever, ordering (close before write), TOCTOU, active-child guard, stale-parent guardabandonSubtaskWithIdwebview message handler: happy path + error catch/logAPI.clearCurrentTaskandAPI.abandonSubtaskpassthroughplusButtonClickedcommand handler callsevictCurrentTaskTest Procedure
new_taskto delegate to a subtask, then navigate away (interrupting the subtask). In History, open the parent — it should show a "Delegated" badge and an "Abandon Subtask" button. Confirm the dialog; the parent returns to active and the subtask is no longer linked.pnpm --filter zoo-code testPre-Submission Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Tests