Tidy up following ClineProvider refactor #2182

diarmidmackenzie · 2025-04-01T13:54:32Z

Context

PR #2149 split ClineProvider into two files.

This required making certain private properties of the ClineProvider class public.

For this PR, I have carefully reviewed all of those, and tidied up everything I can to make the public interface of ClineProvider as clean as possible.

Implementation

Approach:

worked through each property that had to be made public in PR Split webviewMessageHandler out of ClineProvider.ts #2149
reviewed how it was used in webviewMessageHandler, and determined the best solution, which was typically one of:

accept that the property should be public
make the property public, but read-only
adjust the code in webviewMessageHandler to use an alternative public method (in some cases, an appropriate public method didn't exist, but it was sensibe to create it).

For specific details, see line-by-line comments in the code review.

Screenshots

N/A

How to Test

As with previous refactoring, given that the TS compiles cleanly, risk seems low.

No obvious targeted testing that would be valuable. The changes are wide-ranging, so a lot of code paths will be altered, but all in pretty minimal ways that should not impact function.

Get in Touch

@diarmidm on Discord, or the Cline.ts modularization Discord channel.

Important

Refactor ClineProvider to improve encapsulation and state management, adjusting public interfaces and utilizing contextProxy for cleaner code.

Encapsulation:
- Made view, workspaceTracker, and mcpHub private or read-only in ClineProvider.
- Added getMcpHub() in ClineProvider to access mcpHub.
Refactoring:
- Replaced outputChannel.appendLine with log() in ClineProvider and webviewMessageHandler.
- Utilized contextProxy for state management in webviewMessageHandler.
State Management:
- Updated ExtensionState in ExtensionMessage.ts to use Partial<ExtensionState>.
- Modified mergeExtensionState in ExtensionStateContext.tsx to handle partial state updates.

^{This description was created by}^{for 783138c. It will automatically update as commits are pushed.}

Increase type flexibility so we can directly use postMessageToWebview()

expose new public writeDataToCache() method to commonize code. also decided to leave updateApiConfiguration as public, since widely used in diverse ways.

Symmetric naming of functions that complement each other.

Stop using deprecated (private) method. Use contextProxy set/getValue methods instead. Utility functions provided for brevity.

log is already public, outputChannel should be private. But also prefer log internally for the additional value of console logging, plus code brevity.

changeset-bot · 2025-04-01T13:54:36Z

⚠️ No Changeset found

Latest commit: e506dcf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

diarmidmackenzie · 2025-04-01T13:55:02Z

src/core/webview/ClineProvider.ts

 import { ExtensionMessage } from "../../shared/ExtensionMessage"
 import { Mode, PromptComponent, defaultModeSlug, getModeBySlug, getGroupName } from "../../shared/modes"
-import { EXPERIMENT_IDS, experiments as Experiments, experimentDefault, ExperimentId } from "../../shared/experiments"
+import { experimentDefault } from "../../shared/experiments"


Imports ahould have been removed in previous PR, but I overlooked them

diarmidmackenzie · 2025-04-01T13:56:28Z

src/core/webview/ClineProvider.ts

-	// not private, so it can be accessed from webviewMessageHandler
-	// callers could update to get viewLaunched() getter function
-	isViewLaunched = false
+	private view?: vscode.WebviewView | vscode.WebviewPanel


Removed use of this by changing code to use proper public API: postMessageToWebview

diarmidmackenzie · 2025-04-01T13:58:27Z

src/core/webview/ClineProvider.ts

-	latestAnnouncementId = "mar-30-2025-3-11" // update for v3.11.0 announcement
-	// not private, so it can be accessed from webviewMessageHandler
-	settingsImportedAt?: number
+	private _workspaceTracker?: WorkspaceTracker // workSpaceTracker read-only for access outside this class


workspaceTracker needs to be public to allow access to initializeFilePaths() but we can make it readonly externally.

can't apply the TS readonly property because we need to write to this field in our dispose() method, so this is the next best option in terms of limiting public access.

diarmidmackenzie · 2025-04-01T14:00:33Z

src/core/webview/ClineProvider.ts

+	public get workspaceTracker(): WorkspaceTracker | undefined {
+		return this._workspaceTracker
+	}
+	protected mcpHub?: McpHub // Change from private to protected


Switched back to "protected" as code was before. I don't understand the value of this being "protected" as I don't see any sub-classes of ClineProvider, but pre-existing comment shows this is intentional.

Could consider switching to same model as workspaceTracker with an internal property _mcpHub and a getter so that external components can access provider.mcpHub rather than provider,getMcpHub().

I think that's a nicer API to offer, but it's orthogonal to this refactor, so I've not made that change.

diarmidmackenzie · 2025-04-01T14:01:53Z

src/core/webview/ClineProvider.ts

+		return this._workspaceTracker
+	}
+	protected mcpHub?: McpHub // Change from private to protected
+


Reordered properties so they are grouped by access privileges.

These ones are accessed one-time in webviewMessageHandler. Simplest solution is to make them public (and readonly for the ones that don't need to be writeable)

diarmidmackenzie · 2025-04-01T14:02:55Z

src/core/webview/ClineProvider.ts

 		readonly context: vscode.ExtensionContext,
-		// not private, so it can be accessed from webviewMessageHandler
-		readonly outputChannel: vscode.OutputChannel,
+		private readonly outputChannel: vscode.OutputChannel,


Made this private again.

Prefer provider.log() over provider.outputChannel.appendLine()

diarmidmackenzie · 2025-04-01T14:04:14Z

src/core/webview/ClineProvider.ts

 		super()

-		this.outputChannel.appendLine("ClineProvider instantiated")
+		this.log("ClineProvider instantiated")


Made this change throughout this module, as well as in webviewMessageHandler - it's more concise and cleaner.

Minor functional change in that we also log to console, but volume will be minimal, and probably useful to log to console as well as to the outputChannel.

diarmidmackenzie · 2025-04-01T14:04:53Z

src/core/webview/ClineProvider.ts

 		telemetryService.setProvider(this)

-		this.workspaceTracker = new WorkspaceTracker(this)
+		this._workspaceTracker = new WorkspaceTracker(this)


See above - internal field is now _workspaceTracker.

workspaceTracker is public, but read-only via a getter.

diarmidmackenzie · 2025-04-01T14:06:14Z

src/core/webview/ClineProvider.ts

 	}

-	// not private, so it can be accessed from webviewMessageHandler
+	async writeModelsToCache<T>(filename: string, data: T) {


New function, avoids need to make ensureCacheDirectoryExists public, and offers a higher level of abstraction to message handlers.

Symmetry with pre-existing readModelsFromCache function.

Maybe in the future we can remove file management from this file.

diarmidmackenzie · 2025-04-01T14:06:55Z

src/core/webview/ClineProvider.ts

 	// @deprecated - Use `ContextProxy#setValue` instead.
-	// not private, so it can be accessed from webviewMessageHandler
-	async updateGlobalState<K extends keyof GlobalState>(key: K, value: GlobalState[K]) {
+	private async updateGlobalState<K extends keyof GlobalState>(key: K, value: GlobalState[K]) {


Re-instate private status here. message handlers use ContextProxy.setValue as per comment.

diarmidmackenzie · 2025-04-01T14:07:10Z

src/core/webview/ClineProvider.ts

 	// @deprecated - Use `ContextProxy#getValue` instead.
-	// not private, so it can be accessed from webviewMessageHandler
-	getGlobalState<K extends keyof GlobalState>(key: K) {
+	private getGlobalState<K extends keyof GlobalState>(key: K) {


Re-instate private status here. message handlers use ContextProxy.getValue as per comment.

diarmidmackenzie · 2025-04-01T14:07:39Z

src/core/webview/webviewMessageHandler.ts

 import { checkoutDiffPayloadSchema, checkoutRestorePayloadSchema, WebviewMessage } from "../../shared/WebviewMessage"
 import { checkExistKey } from "../../shared/checkExistApiConfig"
-import { EXPERIMENT_IDS, experiments as Experiments, experimentDefault, ExperimentId } from "../../shared/experiments"
+import { EXPERIMENT_IDS, experimentDefault, ExperimentId } from "../../shared/experiments"


Superfluous imports - overlooked in previous PR.

diarmidmackenzie · 2025-04-01T14:07:53Z

src/core/webview/webviewMessageHandler.ts

 import { TelemetrySetting } from "../../shared/TelemetrySetting"
 import { getWorkspacePath } from "../../utils/path"
-import { Mode, PromptComponent, defaultModeSlug, getModeBySlug, getGroupName } from "../../shared/modes"
+import { Mode, defaultModeSlug, getModeBySlug, getGroupName } from "../../shared/modes"


Superfluous import - overlooked in previous PR

diarmidmackenzie · 2025-04-01T14:09:56Z

src/core/webview/webviewMessageHandler.ts

+import { GlobalState } from "../../schemas"

 export const webviewMessageHandler = async (provider: ClineProvider, message: WebviewMessage) => {
+	// Utility functions provided for concise get/update of global state via contextProxy API.


provider.get/updateGlobalState is marked as deprecated in ClineProvider.ts, so don't use it.

getGobalState is more concise than provider.contextProxy.getValue (and used a lot throughout this file, so worthwhile to offer a more concise syntax).

diarmidmackenzie · 2025-04-01T14:10:48Z

src/core/webview/webviewMessageHandler.ts

 			// Load custom modes first
 			const customModes = await provider.customModesManager.getCustomModes()
-			await provider.updateGlobalState("customModes", customModes)
+			await updateGlobalState("customModes", customModes)


Uses new local updateGlobalState utility function, over (deprecated, private) provider.updateGlobalState method. You'll see this change many times in this file.

diarmidmackenzie · 2025-04-01T14:11:14Z

src/core/webview/webviewMessageHandler.ts


 			// If MCP Hub is already initialized, update the webview with current server list
-			if (provider.mcpHub) {
+			const mcpHub = provider.getMcpHub()


provider.mcpHub -> protected again. This is the public method to access it.

diarmidmackenzie · 2025-04-01T14:12:46Z

src/core/webview/webviewMessageHandler.ts

 				})
 			}

-			const cacheDir = await provider.ensureCacheDirectoryExists()


This function is folded into the new higher-level writeModelsToCache() method.

diarmidmackenzie · 2025-04-01T14:13:32Z

src/core/webview/webviewMessageHandler.ts

-						path.join(cacheDir, GlobalFileNames.openRouterModels),
-						JSON.stringify(openRouterModels),
-					)
+					await provider.writeModelsToCache(GlobalFileNames.openRouterModels, openRouterModels)


One of several instances where we have moved to the new higher-level writeModelsToCache() method, alowing provider.ensureCacheDirectoryExists() to become private again.

diarmidmackenzie · 2025-04-01T14:15:24Z

src/core/webview/webviewMessageHandler.ts

 				const currentState = await provider.getState()
 				const stateWithPrompts = { ...currentState, customModePrompts: updatedPrompts }
-				provider.view?.webview.postMessage({ type: "state", state: stateWithPrompts })
+				provider.postMessageToWebview({ type: "state", state: stateWithPrompts })


Use public API, rather than accessing private view property. Had to slightly adjust types on postMessageToWebview to enable this - see later.

That might be why the code had been written this way previously.

Change of plan - use provider.getStateToPostToWebview() instead, then no type relaxation needed.

diarmidmackenzie · 2025-04-01T14:16:58Z

src/shared/ExtensionMessage.ts

 		| "didBecomeVisible"
 	invoke?: "newChat" | "sendMessage" | "primaryButtonClick" | "secondaryButtonClick" | "setChatBoxMessage"
-	state?: ExtensionState
+	state?: Partial<ExtensionState>


Relaxed type here, so that we could move away from using private view property.

See: https://github.com/RooVetGit/Roo-Code/pull/2182/files#r2022956143

Change of plan - not relaxing type after all.

webview-ui/src/context/ExtensionStateContext.tsx

bramburn · 2025-04-01T19:41:08Z

src/core/webview/ClineProvider.ts

 				x.dispose()
 			}
 		}



I wondered 🤔 why there were areas with output channels and this.log
We also need to remove the console.log to this.log in the future

bramburn · 2025-04-01T19:42:24Z

src/core/webview/ClineProvider.ts

 	}

-	// not private, so it can be accessed from webviewMessageHandler
+	async writeModelsToCache<T>(filename: string, data: T) {


Maybe in the future we can remove file management from this file.

webview-ui/src/context/ExtensionStateContext.tsx

Add Korean language

bramburn · 2025-04-05T23:02:21Z

Nice 👍

…

On Sat, 5 Apr 2025, 23:00 Matt Rubens, ***@***.***> wrote: Merged #2182 <#2182> into main. — Reply to this email directly, view it on GitHub <#2182 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUTT3O37AT575Y6LTFBB6D2YBHAZAVCNFSM6AAAAAB2G7RKY6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJXGE2DCNBUGU3TAMQ> . You are receiving this because you commented.Message ID: ***@***.***>

diarmidmackenzie added 11 commits April 1, 2025 10:55

Remove unused imports

787405a

Restore protected status for mcpHub property

3de5d1a

More mcpHub tidy-up

18fb2e2

Remove need for access to private "view" property

55773ec

Increase type flexibility so we can directly use postMessageToWebview()

Make ensureCacheDirectoryExists private again

ac44bdb

expose new public writeDataToCache() method to commonize code. also decided to leave updateApiConfiguration as public, since widely used in diverse ways.

Match naming of readModelsFromCache.

cd8fa36

Symmetric naming of functions that complement each other.

Decision: Leave readModelsFromCache as public

2fb658c

Make updateGlobalState and setGlobalState private again

fb20cb3

Stop using deprecated (private) method. Use contextProxy set/getValue methods instead. Utility functions provided for brevity.

Add clarifying comment to explain presence of utility functions

d8aba89

Prefer this/provider.log over outputChannel.appendLine

1521e95

log is already public, outputChannel should be private. But also prefer log internally for the additional value of console logging, plus code brevity.

Lock down access to properties as far as reasonably possible

783138c

github-project-automation bot added this to Roo Code Roadmap Apr 1, 2025

github-project-automation bot moved this to New in Roo Code Roadmap Apr 1, 2025

diarmidmackenzie commented Apr 1, 2025

View reviewed changes

webview-ui/src/context/ExtensionStateContext.tsx Outdated Show resolved Hide resolved

diarmidmackenzie changed the title ~~Tidy cline provider refactor~~ Tidy up following ClineProvider refactor Apr 1, 2025

diarmidmackenzie marked this pull request as ready for review April 1, 2025 14:46

diarmidmackenzie requested review from cte and mrubens as code owners April 1, 2025 14:46

dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 1, 2025

bramburn approved these changes Apr 1, 2025

View reviewed changes

hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap Apr 1, 2025

PR feedback - don't relax type checks

8917589

monotykamary pushed a commit to monotykamary/Roo-Code that referenced this pull request Apr 4, 2025

Merge pull request RooCodeInc#2182 from jiyeonseo/main

6b9c2a1

Add Korean language

mrubens approved these changes Apr 5, 2025

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 5, 2025

Merge branch 'main' into tidy-cline-provider-refactor

e506dcf

mrubens merged commit 7661ef9 into RooCodeInc:main Apr 5, 2025
12 checks passed

github-project-automation bot moved this from PR [Pre Approval Review] to Done in Roo Code Roadmap Apr 5, 2025

Tidy up following ClineProvider refactor #2182

Tidy up following ClineProvider refactor #2182

Uh oh!

Conversation

diarmidmackenzie commented Apr 1, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Implementation

Screenshots

How to Test

Get in Touch

Uh oh!

changeset-bot bot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diarmidmackenzie Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bramburn commented Apr 5, 2025 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

diarmidmackenzie commented Apr 1, 2025 •

edited by ellipsis-dev bot

Loading

changeset-bot bot commented Apr 1, 2025 •

edited

Loading

diarmidmackenzie Apr 1, 2025 •

edited

Loading