Conversation
📝 WalkthroughWalkthroughIntroduces ChangesTaskRegistry migration
Estimated code review effort: 4 (Complex) | ~45 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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/task/__tests__/TaskRegistry.spec.ts (1)
8-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDownstream effect of the invariant-testability gap flagged on
TaskRegistry.ts.This helper is reused by every test in the suite but only checks
taskIds.length === registry.length(tautological, both derived fromstack) plus per-idgetByIdlookups — it never compares againsttasks.size, so it can't catch a map/stack desync. See the companion comment onsrc/core/task/TaskRegistry.ts(lines 3-11) for the root cause and suggested fix.🤖 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/task/__tests__/TaskRegistry.spec.ts` around lines 8 - 17, Update the assertInvariant helper to compare registry.taskIds.length against registry.tasks.size, rather than registry.length, so the invariant detects stack/map desynchronization; retain the existing per-ID and current-task checks.src/core/task/TaskRegistry.ts (1)
3-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdvertised dual-write invariant isn't externally verifiable.
The docstring claims
tasks.size === stack.length at all times, but no public accessor exposestasks.sizeindependently ofstack.length(lengthandtaskIdsare both derived fromstack). Nothing outside the class — including its own test suite (TaskRegistry.spec.ts'sassertInvariant, lines 8-17) — can actually detect a future regression wheretasksgrows out of sync withstack(e.g., an orphaned map entry). The implementation is correct today, but the invariant is currently unfalsifiable by tests.Consider exposing a minimal size accessor (or having
assertInvariantreach into the private map via(registry as any).tasks.size) so the invariant this class explicitly documents can actually be asserted.♻️ Example fix (test-side, no production API change)
function assertInvariant(registry: TaskRegistry) { const ids = registry.taskIds expect(ids.length).toBe(registry.length) + expect((registry as any).tasks.size).toBe(registry.length) for (const id of ids) { expect(registry.getById(id)).toBeDefined() }🤖 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/task/TaskRegistry.ts` around lines 3 - 11, Make the documented TaskRegistry invariant externally testable by updating TaskRegistry.spec.ts's assertInvariant to read the private tasks map size via the test-only cast and compare it with stack length, without changing the production API. Keep the existing checks for taskIds and length, and ensure orphaned map entries cause the assertion to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/task/__tests__/TaskRegistry.spec.ts`:
- Around line 8-17: Update the assertInvariant helper to compare
registry.taskIds.length against registry.tasks.size, rather than
registry.length, so the invariant detects stack/map desynchronization; retain
the existing per-ID and current-task checks.
In `@src/core/task/TaskRegistry.ts`:
- Around line 3-11: Make the documented TaskRegistry invariant externally
testable by updating TaskRegistry.spec.ts's assertInvariant to read the private
tasks map size via the test-only cast and compare it with stack length, without
changing the production API. Keep the existing checks for taskIds and length,
and ensure orphaned map entries cause the assertion to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a190bacb-aa39-4f90-86bd-d6522abd5751
📒 Files selected for processing (8)
src/__tests__/helpers/provider-stub.tssrc/__tests__/removeClineFromStack-delegation.spec.tssrc/__tests__/single-open-invariant.spec.tssrc/core/task/TaskRegistry.tssrc/core/task/__tests__/TaskRegistry.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.tssrc/core/webview/__tests__/ClineProvider.spec.ts
github-actions
Bot
added
the
awaiting-review
| it("removes the focused task, aborts it, and clears listeners", async () => { | ||
| const { provider, childTask } = buildMockProvider({ childTaskId: "child-1" }) | ||
| expect(provider.clineStack).toHaveLength(1) | ||
| expect((provider as any).taskRegistry.length).toBe(1) |
There was a problem hiding this comment.
is it possible to not use any for assertions?
2 participants
Related GitHub Issue
Closes: #367
Description
Replaces the
clineStack: Task[]array inClineProviderwith aTaskRegistryadapter (Expand-Contract migration, Story 3.2a of Epic 3 / Issue #367).Key implementation details:
TaskRegistrywraps aMap<string, Task>(O(1) lookup) and a parallelstring[]stack (insertion order). Invariant:tasks.size === stack.lengthat all times.clineStackcall sites inClineProvider.tsare migrated; external behaviour is identical atmaxConcurrency=1.removeClineFromStack()now removestaskRegistry.currentby ID rather than blindly popping the top, fixing a latent bug where focus and top-of-stack could diverge.createTaskWithHistoryItem) uses a newreplace()method that swaps the task object in-place, preserving both stack index and focus pointer.getRunning(),hasRunning(),setCurrent()) are ready for Epic 3's later stories (TaskScheduler, showTaskWithId split).Reviewers should pay attention to:
TaskRegistry.replace()— used only in the rehydration path; guards against unknown and duplicate IDs.pop()— only resets_currentTaskIdwhen the popped entry was the current focus (not always the top).Test Procedure
TaskRegistryoperations and the dual-write invariant:src/core/task/__tests__/TaskRegistry.spec.tsremoveClineFromStack-delegation.spec.ts,single-open-invariant.spec.ts,ClineProvider.spec.ts,ClineProvider.flicker-free-cancel.spec.tspnpm exec vitest runfromsrc/— 7006 tests passingPre-Submission Checklist
Documentation Updates
Additional Notes
This is a pure adapter migration — no behaviour change at current concurrency settings. The registry surface area (
getRunning,hasRunning,setCurrent) is used by follow-on stories in Epic 3 but is otherwise dormant in this PR.Summary by CodeRabbit
Improvements
Bug Fixes