Refactor core monolith helpers#27
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts API conversation message construction into prepareApiConversationMessage and introduces PendingEditOperationStore for pending-edit lifecycle; Task and ClineProvider now delegate to these new modules and accompanying tests were added. ChangesAPI Conversation History Extraction
Pending Edit Operation Store
Sequence Diagram(s)sequenceDiagram
participant Task
participant prepareApiConversationMessage
participant apiConversationHistory
participant ClineProvider
participant PendingEditOperationStore
Task->>prepareApiConversationMessage: prepareApiConversationMessage(message, api/context)
prepareApiConversationMessage-->>Task: ApiMessage (ts,id,content changes)
Task->>apiConversationHistory: push ApiMessage
ClineProvider->>PendingEditOperationStore: set(operationId, PendingEditOperationInput)
PendingEditOperationStore-->>ClineProvider: ack (stored, timeout scheduled)
Note over PendingEditOperationStore: timeout -> auto-clear -> log
ClineProvider->>PendingEditOperationStore: clear/clearAll (explicit)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Nice - thanks for taking this on - couple comments based on some opportunities we could take with this refactor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/task/__tests__/apiConversationHistory.spec.ts (1)
5-107: 💤 Low valueConsider adding edge case coverage.
The core paths are well tested. A few additional cases could strengthen coverage:
- User message when last effective isn't an assistant (verifies
tool_result→textconversion on lines 112-124)- Anthropic protocol with reasoning but missing
thoughtSignature(falls through to generic reasoning block)🤖 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__/apiConversationHistory.spec.ts` around lines 5 - 107, Add two edge-case tests to src/core/task/__tests__/apiConversationHistory.spec.ts for prepareApiConversationMessage: (1) a user message containing a tool_result when the last effective message in apiConversationHistory is not an assistant (e.g., it's a user or system) and assert the tool_result is converted into a text block (type "text") and ts set to Date.now(); (2) an Anthropic apiConfiguration (apiProvider: "anthropic") where the api object does not provide getThoughtSignature but reasoning is present, and assert the function falls back to emitting a generic reasoning block (e.g., { type: "reasoning", text: ... }) rather than a thinking block — reference prepareApiConversationMessage, getThoughtSignature, tool_result and tool_use to locate the relevant logic.
🤖 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 `@src/core/webview/PendingEditOperationStore.ts`:
- Around line 30-33: The store is saving and returning the images array by
reference which allows external mutation; when calling operations.set with
operationId and editData (and when creating timeoutId/createdAt) clone the
images array (e.g., replace editData.images with a shallow copy like
[...editData.images] or map-clone items) so the internal store holds its own
copy, and likewise when exposing stored operations (the getters that read from
this.operations) return copies of the images array instead of the original
reference so callers cannot mutate internal state; update the set/save path and
the accessors that return stored operation objects to perform these copies for
the images field.
---
Nitpick comments:
In `@src/core/task/__tests__/apiConversationHistory.spec.ts`:
- Around line 5-107: Add two edge-case tests to
src/core/task/__tests__/apiConversationHistory.spec.ts for
prepareApiConversationMessage: (1) a user message containing a tool_result when
the last effective message in apiConversationHistory is not an assistant (e.g.,
it's a user or system) and assert the tool_result is converted into a text block
(type "text") and ts set to Date.now(); (2) an Anthropic apiConfiguration
(apiProvider: "anthropic") where the api object does not provide
getThoughtSignature but reasoning is present, and assert the function falls back
to emitting a generic reasoning block (e.g., { type: "reasoning", text: ... })
rather than a thinking block — reference prepareApiConversationMessage,
getThoughtSignature, tool_result and tool_use to locate the relevant logic.
🪄 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: 13150e62-f799-489b-9392-bccec80d5314
📒 Files selected for processing (4)
src/core/task/__tests__/apiConversationHistory.spec.tssrc/core/task/apiConversationHistory.tssrc/core/webview/PendingEditOperationStore.tssrc/core/webview/__tests__/PendingEditOperationStore.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/core/webview/tests/PendingEditOperationStore.spec.ts
|
Addressed the reasoning payload-shape comment in Validation: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
edelauna
force-pushed
the
issue-8-monolith-refactor
branch
from
cfd697e to
554ad63
Compare
edelauna
requested review from
JamesRobert20,
navedmerchant and
taltas
as code owners
|
@doctarock if you can get this merged - then I dont have to manually approve workflow runs for your PRs since you'll no longer be a first time contributor. |
edelauna
force-pushed
the
issue-8-monolith-refactor
branch
from
5cbe5b2 to
1f5bbb4
Compare
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for adding the commit - on re-review had one followup comment.
| getResponseId?: () => string | undefined | ||
| getEncryptedContent?: () => { encrypted_content: string; id?: string } | undefined | ||
| getThoughtSignature?: () => string | undefined | ||
| getReasoningDetails?: () => any[] | undefined |
There was a problem hiding this comment.
The original Task.ts code called handler.getSummary?.() and fell back to [] — was that method intentionally dropped here because no handler ever returns a non-empty summary? If so, might be worth a brief comment so a future reader knows it was deliberate rather than an accidental omission.
There was a problem hiding this comment.
getSummary was intentionally omitted because no provider implements it and the preserved wire shape is the explicit summary: [] fallback, it does deserve a comment though
edelauna
force-pushed
the
issue-8-monolith-refactor
branch
from
24574f4 to
e9453bf
Compare
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for doing this, and also documenting it out via issues.
2 participants
Related GitHub Issue
Refs #8
Description
Extract two low-coupling pieces from the core monolith files while preserving the existing call flow:
Task.tsintosrc/core/task/apiConversationHistory.tsClineProvider.tsintosrc/core/webview/PendingEditOperationStore.tsTaskandClineProviderkeep orchestrating the same operations, but the focused logic now lives in named collaborators.Test Procedure
pnpm --dir src check-typespnpm --dir src exec vitest run core/task/__tests__/reasoning-preservation.test.ts core/task/__tests__/grounding-sources.test.ts core/webview/__tests__/checkpointRestoreHandler.spec.tsNotes
This is intentionally scoped to the two extraction candidates called out in #8.
Summary by CodeRabbit
Refactor
Bug Fixes
Tests