refactor(task): extract checkpoint manager#106
Conversation
doctarock
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracts checkpoint management into a new CheckpointManager and runtime abstraction. Checkpoint helper functions (save/restore/diff, checkGitInstallation, get service) now accept CheckpointRuntime; Task delegates checkpoint state and operations through the manager and initializes its service during startup. ChangesCheckpoint Manager Extraction
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/task/Task.tsOops! Something went wrong! :( ESLint: 9.28.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/checkpoints/index.ts (1)
19-36: ⚡ Quick winWeakly typed runtime/context interface loses type safety in checkpoint helpers.
CheckpointTaskContextusesanyforproviderRef,clineMessages,say, andcombineMessages. The whole point of introducing an abstraction was to formalize the contract, but the current shape forfeits compile-time checks at every call site:
task.providerRef.deref()?.postMessageToWebview(...)is untyped, so typos in message types/payloads (e.g.,currentCheckpointUpdated,checkpointInitWarning) won't be flagged.task.clineMessages.filter(({ say }) => say === "checkpoint_saved").map(({ text }) => text!)(Line 414) losesClineMessagefield narrowing, and thetext!non-null assertion can silently hide nulls.task.say("checkpoint_saved", to, undefined, undefined, { from, to, suppressMessage: !!suppressMessage }, ...)(Lines 258-269) won't be validated against the actualsay(type: ClineSay, ...)signature, so futureClineSayadditions/removals or argument order changes will not surface here.Consider replacing
anywith the existing concrete types (ClineMessage,ClineSay, a narrowedClineProviderinterface for the methods used —postMessageToWebview,log,cancelTask,context.globalStorageUri). The abstraction would still decouple from the fullTaskclass while preserving end-to-end type checking.♻️ Sketch of tighter typing
-export type CheckpointTaskContext = { - taskId: string - cwd?: string - providerRef: WeakRef<any> - clineMessages: any[] - messageManager: { - rewindToTimestamp: (ts: number, options: { includeTargetMessage: boolean }) => Promise<void> - } - say: (...args: any[]) => Promise<any> - combineMessages: (messages: any[]) => any -} +import type { ClineMessage, ClineSay } from "@roo-code/types" + +export interface CheckpointProvider { + postMessageToWebview: (msg: unknown) => Promise<void> | void + log?: (message: string) => void + cancelTask?: () => void + context: { globalStorageUri: { fsPath: string } } +} + +export type CheckpointTaskContext = { + taskId: string + cwd?: string + providerRef: WeakRef<CheckpointProvider> + clineMessages: ClineMessage[] + messageManager: { + rewindToTimestamp: (ts: number, options: { includeTargetMessage: boolean }) => Promise<void> + } + say: (type: ClineSay, ...args: unknown[]) => Promise<unknown> + combineMessages: (messages: ClineMessage[]) => ClineMessage[] +}🤖 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/checkpoints/index.ts` around lines 19 - 36, Replace the loose any types in CheckpointTaskContext/CheckpointRuntime with concrete interfaces: change providerRef: WeakRef<any> to WeakRef<ClineProvider> (ClineProvider exposing postMessageToWebview, log, cancelTask, context.globalStorageUri), clineMessages: any[] to ClineMessage[], say: (...args: any[]) => Promise<any> to say: (type: ClineSay, ...args: any[]) => Promise<any>, and combineMessages: (messages: any[]) => any to combineMessages: (messages: ClineMessage[]) => CombinedMessageType; update referenced helpers that call task.providerRef.deref(), task.clineMessages.filter(...), and task.say(...) to use these types so TypeScript validates message names/payloads, removes unsafe non-null assertions, and preserves the abstraction from the full Task class.
🤖 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/checkpoints/index.ts`:
- Around line 19-36: Replace the loose any types in
CheckpointTaskContext/CheckpointRuntime with concrete interfaces: change
providerRef: WeakRef<any> to WeakRef<ClineProvider> (ClineProvider exposing
postMessageToWebview, log, cancelTask, context.globalStorageUri), clineMessages:
any[] to ClineMessage[], say: (...args: any[]) => Promise<any> to say: (type:
ClineSay, ...args: any[]) => Promise<any>, and combineMessages: (messages:
any[]) => any to combineMessages: (messages: ClineMessage[]) =>
CombinedMessageType; update referenced helpers that call
task.providerRef.deref(), task.clineMessages.filter(...), and task.say(...) to
use these types so TypeScript validates message names/payloads, removes unsafe
non-null assertions, and preserves the abstraction from the full Task class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 107d8c5e-ec1f-47ff-aa30-1dc732735760
📒 Files selected for processing (2)
src/core/checkpoints/index.tssrc/core/task/Task.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
1 participant
Related GitHub Issue
Closes #19
Part of #8
Description
Extracts checkpoint state and lifecycle ownership from
Task.tsinto a dedicatedCheckpointManagerboundary insrc/core/checkpoints.CheckpointManagerto own checkpoint enablement, timeout, service instance, and initialization stateTaskdependency from checkpoint helpers by introducing a smaller checkpoint runtime/context interfaceTaskpublic checkpoint methods and compatibility accessors in place while delegating save/restore/diff/init to the managerTest Procedure
.\.corepack-shims\pnpm.cmd --dir src check-types.\.corepack-shims\pnpm.cmd --filter ./src exec vitest run core/checkpoints/__tests__/checkpoint.test.ts core/webview/__tests__/checkpointRestoreHandler.spec.tsgit pushhook ran repo check-types successfullyNotes
No changeset added because this is an internal refactor with no user-facing package/API change.
Summary by CodeRabbit