Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/__tests__/migrateSettings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,18 @@ describe("Settings Migration", () => {
// Mock extension context
mockContext = {
globalStorageUri: { fsPath: mockStoragePath },
globalState: {
get: jest.fn((key: string) => {
if (key === "clineCustomModes") {
return "some old custom modes content"
}
if (key === "customInstructions") {
return undefined
}
return undefined
}),
update: jest.fn().mockResolvedValue(undefined),
},
} as unknown as vscode.ExtensionContext

// Set global outputChannel for all tests
Expand Down
1 change: 1 addition & 0 deletions src/core/prompts/__tests__/custom-system-prompt.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const mockedFs = fs as jest.Mocked<typeof fs>

// Mock the fileExistsAtPath function
jest.mock("../../../utils/fs", () => ({
...(jest.requireActual("../../../utils/fs") as object), // Spread all original exports
fileExistsAtPath: jest.fn().mockResolvedValue(true),
createDirectoriesForFile: jest.fn().mockResolvedValue([]),
}))
Expand Down
17 changes: 1 addition & 16 deletions src/core/prompts/sections/custom-instructions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,7 @@ import { Dirent } from "fs"
import { isLanguage } from "@roo-code/types"

import { LANGUAGES } from "../../../shared/language"

/**
* Safely read a file and return its trimmed content
*/
async function safeReadFile(filePath: string): Promise<string> {
try {
const content = await fs.readFile(filePath, "utf-8")
return content.trim()
} catch (err) {
const errorCode = (err as NodeJS.ErrnoException).code
if (!errorCode || !["ENOENT", "EISDIR"].includes(errorCode)) {
throw err
}
return ""
}
}
import { safeReadFile } from "../../../utils/fs"

/**
* Check if a directory exists
Expand Down
19 changes: 1 addition & 18 deletions src/core/prompts/sections/custom-system-prompt.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import fs from "fs/promises"
import path from "path"
import { Mode } from "../../../shared/modes"
import { fileExistsAtPath } from "../../../utils/fs"
import { fileExistsAtPath, safeReadFile } from "../../../utils/fs"

export type PromptVariables = {
workspace?: string
Expand All @@ -25,23 +25,6 @@ function interpolatePromptContent(content: string, variables: PromptVariables):
return interpolatedContent
}

/**
* Safely reads a file, returning an empty string if the file doesn't exist
*/
async function safeReadFile(filePath: string): Promise<string> {
try {
const content = await fs.readFile(filePath, "utf-8")
// When reading with "utf-8" encoding, content should be a string
return content.trim()
} catch (err) {
const errorCode = (err as NodeJS.ErrnoException).code
if (!errorCode || !["ENOENT", "EISDIR"].includes(errorCode)) {
throw err
}
return ""
}
}

/**
* Get the path to a system prompt file for a specific mode
*/
Expand Down
1 change: 1 addition & 0 deletions src/core/task/__tests__/Task.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ jest.mock("../../../utils/storage", () => ({
}))

jest.mock("../../../utils/fs", () => ({
...(jest.requireActual("../../../utils/fs") as object), // Spread all original exports
fileExistsAtPath: jest.fn().mockImplementation((filePath) => {
return filePath.includes("ui_messages.json") || filePath.includes("api_conversation_history.json")
}),
Expand Down
40 changes: 29 additions & 11 deletions src/core/webview/ClineProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import { Package } from "../../shared/package"
import { findLast } from "../../shared/array"
import { supportPrompt } from "../../shared/support-prompt"
import { GlobalFileNames } from "../../shared/globalFileNames"
import { GlobalContentIds } from "../../shared/globalContentIds"
import { ExtensionMessage } from "../../shared/ExtensionMessage"
import { Mode, defaultModeSlug } from "../../shared/modes"
import { experimentDefault, experiments, EXPERIMENT_IDS } from "../../shared/experiments"
Expand Down Expand Up @@ -65,6 +66,7 @@ import { webviewMessageHandler } from "./webviewMessageHandler"
import { WebviewMessage } from "../../shared/WebviewMessage"
import { EMBEDDING_MODEL_PROFILES } from "../../shared/embeddingModels"
import { ProfileValidator } from "../../shared/ProfileValidator"
import { ContentManager } from "../../services/content/ContentManager"

/**
* https://github.com/microsoft/vscode-webview-ui-toolkit-samples/blob/main/default/weather-webview/src/providers/WeatherViewProvider.ts
Expand Down Expand Up @@ -101,6 +103,7 @@ export class ClineProvider
return this._workspaceTracker
}
protected mcpHub?: McpHub // Change from private to protected
public contentManager: ContentManager // Declare private member

public isViewLaunched = false
public settingsImportedAt?: number
Expand Down Expand Up @@ -147,6 +150,14 @@ export class ClineProvider
.catch((error) => {
this.log(`Failed to initialize MCP Hub: ${error}`)
})

// Instantiate ContentManager
this.contentManager = new ContentManager({
log: this.log.bind(this),
postMessageToWebview: this.postMessageToWebview.bind(this),
postStateToWebview: this.postStateToWebview.bind(this),
globalStorageUriFsPath: this.contextProxy.globalStorageUri.fsPath,
})
}

// Adds a new Cline instance to clineStack, marking the start of a new task.
Expand Down Expand Up @@ -988,10 +999,23 @@ export class ClineProvider
await this.initClineWithHistoryItem({ ...historyItem, rootTask, parentTask })
}

async updateCustomInstructions(instructions?: string) {
// User may be clearing the field.
await this.updateGlobalState("customInstructions", instructions || undefined)
await this.postStateToWebview()
// Settings Directory

async ensureSettingsDirectoryExists(): Promise<string> {
const { getSettingsDirectoryPath } = await import("../../utils/storage")
const globalStoragePath = this.contextProxy.globalStorageUri.fsPath
return getSettingsDirectoryPath(globalStoragePath)
}

private async getContentPath(contentId: string): Promise<string | undefined> {
const settingsDir = await this.ensureSettingsDirectoryExists()
switch (contentId) {
case GlobalContentIds.customInstructions:
return path.join(settingsDir, GlobalFileNames.customInstructions)
default:
this.log(`Unknown contentId: ${contentId}`)
return undefined
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason to have these methods inside ClineProvider?

If not, it might be a good idea to move them out since this class is already quite big.


// MCP
Expand Down Expand Up @@ -1019,12 +1043,6 @@ export class ClineProvider
return mcpServersDir
}

async ensureSettingsDirectoryExists(): Promise<string> {
const { getSettingsDirectoryPath } = await import("../../utils/storage")
const globalStoragePath = this.contextProxy.globalStorageUri.fsPath
return getSettingsDirectoryPath(globalStoragePath)
}

// OpenRouter

async handleOpenRouterCallback(code: string) {
Expand Down Expand Up @@ -1474,7 +1492,7 @@ export class ClineProvider
return {
apiConfiguration: providerSettings,
lastShownAnnouncementId: stateValues.lastShownAnnouncementId,
customInstructions: stateValues.customInstructions,
customInstructions: await this.contentManager.readContent(GlobalContentIds.customInstructions),
apiModelId: stateValues.apiModelId,
alwaysAllowReadOnly: stateValues.alwaysAllowReadOnly ?? false,
alwaysAllowReadOnlyOutsideWorkspace: stateValues.alwaysAllowReadOnlyOutsideWorkspace ?? false,
Expand Down
19 changes: 18 additions & 1 deletion src/core/webview/webviewMessageHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { RouterName, toRouterName, ModelRecord } from "../../shared/api"
import { supportPrompt } from "../../shared/support-prompt"

import { checkoutDiffPayloadSchema, checkoutRestorePayloadSchema, WebviewMessage } from "../../shared/WebviewMessage"
import { GlobalContentIds } from "../../shared/globalContentIds"
import { checkExistKey } from "../../shared/checkExistApiConfig"
import { experimentDefault } from "../../shared/experiments"
import { Terminal } from "../../integrations/terminal/Terminal"
Expand Down Expand Up @@ -130,7 +131,23 @@ export const webviewMessageHandler = async (provider: ClineProvider, message: We
await provider.initClineWithTask(message.text, message.images)
break
case "customInstructions":
await provider.updateCustomInstructions(message.text)
await provider.contentManager.updateContent(GlobalContentIds.customInstructions, message.text)
break
case "openContent":
if (message.contentId) {
// Check for contentId instead of filePath
await provider.contentManager.openContent(message.contentId)
} else {
console.error("openContent message missing contentId")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling here only logs to console but doesn't provide feedback to the user. When operations fail, users won't know what went wrong.

Consider:

  • Sending error messages back to the webview
  • Showing user-friendly error notifications
  • Adding proper error recovery mechanisms

}
break
case "refreshContent":
if (message.contentId) {
await provider.contentManager.refreshContent(message.contentId)
} else {
// Handle error or log if contentId is missing
console.error("refreshContent message missing contentId")
}
break
case "alwaysAllowReadOnly":
await updateGlobalState("alwaysAllowReadOnly", message.bool ?? undefined)
Expand Down
102 changes: 102 additions & 0 deletions src/services/content/ContentManager.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import * as vscode from "vscode"
import * as path from "path"
import fs from "fs/promises"

import { GlobalContentIds } from "../../shared/globalContentIds"
import { GlobalFileNames } from "../../shared/globalFileNames"
import { openFile } from "../../integrations/misc/open-file"
import { safeReadFile } from "../../utils/fs"
import { getSettingsDirectoryPath } from "../../utils/storage" // Need to confirm this import path
import { ExtensionMessage } from "../../shared/ExtensionMessage" // Assuming this type is needed for postMessageToWebview

// Define an interface for the dependencies ContentManager needs from ClineProvider
interface ContentManagerDependencies {
log: (message: string) => void
postMessageToWebview: (message: ExtensionMessage) => Promise<void>
postStateToWebview: () => Promise<void>
globalStorageUriFsPath: string // To replace contextProxy.globalStorageUri.fsPath
}

export class ContentManager {
private readonly log: (message: string) => void
private readonly postMessageToWebview: (message: ExtensionMessage) => Promise<void>
private readonly postStateToWebview: () => Promise<void>
private readonly globalStorageUriFsPath: string

constructor(dependencies: ContentManagerDependencies) {
this.log = dependencies.log
this.postMessageToWebview = dependencies.postMessageToWebview
this.postStateToWebview = dependencies.postStateToWebview
this.globalStorageUriFsPath = dependencies.globalStorageUriFsPath
}

private async ensureSettingsDirectoryExists(): Promise<string> {
const globalStoragePath = this.globalStorageUriFsPath
return getSettingsDirectoryPath(globalStoragePath)
}

private async getContentPath(contentId: string): Promise<string | undefined> {
const settingsDir = await this.ensureSettingsDirectoryExists()
switch (contentId) {
case GlobalContentIds.customInstructions:
return path.join(settingsDir, GlobalFileNames.customInstructions)
default:
this.log(`Unknown contentId: ${contentId}`)
return undefined
}
}

async openContent(contentId: string): Promise<void> {
const filePath = await this.getContentPath(contentId)

if (!filePath) {
this.log(`File path could not be determined for contentId: ${contentId}`)
return
}

await openFile(filePath, { create: true })
await vscode.commands.executeCommand("workbench.action.files.revert")
}

async refreshContent(contentId: string): Promise<void> {
const content = await this.readContent(contentId)
await this.updateContent(contentId, content)
this.postMessageToWebview({
type: "contentRefreshed",
contentId: contentId,
success: true, // Assuming success for now
})
}

async updateContent(contentId: string, content?: string) {
const filePath = await this.getContentPath(contentId)

if (!filePath) {
this.log(`File path could not be determined for contentId: ${contentId}`)
return
}

if (content && content.trim()) {
await fs.writeFile(filePath, content.trim(), "utf-8")
this.log(`Updated content file: ${filePath}`)
} else {
try {
await fs.unlink(filePath)
this.log(`Deleted content file: ${filePath}`)
} catch (error: any) {
if (error.code !== "ENOENT") {
this.log(`Error deleting content file: ${error.message}`)
throw error
}
}
}
// Update the webview state
await this.postStateToWebview()
}

async readContent(contentId: string): Promise<string> {
const filePath = await this.getContentPath(contentId)

return filePath ? ((await safeReadFile(filePath)) ?? "") : ""
}
}
2 changes: 2 additions & 0 deletions src/shared/ExtensionMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export interface ExtensionMessage {
| "indexingStatusUpdate"
| "indexCleared"
| "codebaseIndexConfig"
| "contentRefreshed"
text?: string
action?:
| "chatButtonClicked"
Expand Down Expand Up @@ -105,6 +106,7 @@ export interface ExtensionMessage {
customMode?: ModeConfig
slug?: string
success?: boolean
contentId?: string
values?: Record<string, any>
requestId?: string
promptText?: string
Expand Down
3 changes: 3 additions & 0 deletions src/shared/WebviewMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export interface WebviewMessage {
| "openImage"
| "openFile"
| "openMention"
| "openContent"
| "cancelTask"
| "updateVSCodeSetting"
| "getVSCodeSetting"
Expand Down Expand Up @@ -118,6 +119,7 @@ export interface WebviewMessage {
| "deleteCustomMode"
| "setopenAiCustomModelInfo"
| "openCustomModesSettings"
| "refreshContent"
| "checkpointDiff"
| "checkpointRestore"
| "deleteMcpServer"
Expand Down Expand Up @@ -170,6 +172,7 @@ export interface WebviewMessage {
modeConfig?: ModeConfig
timeout?: number
payload?: WebViewMessagePayload
contentId?: string
source?: "global" | "project"
requestId?: string
ids?: string[]
Expand Down
3 changes: 3 additions & 0 deletions src/shared/globalContentIds.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const GlobalContentIds = {
customInstructions: "customInstructions" as const,
}
1 change: 1 addition & 0 deletions src/shared/globalFileNames.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@ export const GlobalFileNames = {
uiMessages: "ui_messages.json",
mcpSettings: "mcp_settings.json",
customModes: "custom_modes.yaml",
customInstructions: "custom_instructions.md",
taskMetadata: "task_metadata.json",
}
16 changes: 16 additions & 0 deletions src/utils/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,19 @@ export async function fileExistsAtPath(filePath: string): Promise<boolean> {
return false
}
}

/**
* Safely read a file and return its trimmed content
*/
export async function safeReadFile(filePath: string): Promise<string> {
try {
const content = await fs.readFile(filePath, "utf-8")
return content ? content.trim() : ""
} catch (err) {
const errorCode = (err as NodeJS.ErrnoException).code
if (!errorCode || !["ENOENT", "EISDIR"].includes(errorCode)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error handling pattern for ENOENT is duplicated in multiple places:

  • Here in safeReadFile
  • In ClineProvider.updateContent
  • In custom-instructions.ts
  • In custom-system-prompt.ts

It would be a good idea to consolidate this on a single helper function.

throw err
}
return ""
}
}
Loading
Loading