-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Modes selector improvements #4902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Modes selector improvements #4902
Conversation
…ks it for the first time
vscode.postMessage({ | ||
type: "switchTab", | ||
tab: "marketplace", | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if we could switch directly to the modes tab of the marketplace. We do something similar for links to settings, where we can deep link to a specific settings screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I built a whole thing for this, extending the ExtensionContext and such, but I wasn't too confident of the implementation. I'll take another stab at it, thanks for the pointer.
</div> | ||
</div> | ||
<p className="my-0 text-sm"> | ||
Specialized personas that tailor Roo's behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's internationalize this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had done it but probably messed up staging my changes, the strings are all in the i18n files already, sorry.
public setPeopleProperty(propertyName: string, value: any) { | ||
if (TelemetryClient.telemetryEnabled) { | ||
try { | ||
posthog.people.set({ [propertyName]: value }) | ||
} catch (_error) { | ||
// Silently fail if there's an error setting a property. | ||
} | ||
} | ||
} | ||
|
||
public getPeopleProperty(propertyName: string): any { | ||
if (TelemetryClient.telemetryEnabled) { | ||
try { | ||
return posthog.get_property(propertyName) | ||
} catch (_error) { | ||
// Silently fail if there's an error getting a property. | ||
return null | ||
} | ||
} | ||
return null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can probably exclude these changes from the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is stupid, sorry 🤦
Remnants from my first attempt at tracking whether the user opened the mode selector
…ink from ModeSelector to the Modes subtab
…e in the ModeSelector
…ng built-in ones intact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really nice!
I left a couple of minor suggestions, also can you take a look at the conflicts so I can move this PR for review?
Thank you!
|
||
if (TelemetryService.hasInstance()) { | ||
// Determine which setting was changed by comparing objects | ||
const oldPrompt = existingPrompts[message.promptMode] || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the complexity of this logic and the fact it's repeated elsewhere, but this was Roo's insistent suggestion on how to determine the changed setting and I couldn't figure out a different one which didn't lead to much more complex changes (eg one global event per setting change)
// This could be more generic, but keeping it specific for now | ||
if (message.tab === "modes" && TelemetryService.hasInstance()) { | ||
TelemetryService.instance.captureModesViewShown() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like the idea of a "Tab Shown" event - I might look into replacing this with that quickly.
Description
Modes are one of RooCode's main differentiators – users who try different modes are much more likely to stick around.
But they can be hard to understand and can go unnoticed, especially as people are just getting started with the product.
This PR aims to improve that by:
Test Procedure
The majority of the changes are in the UI, so testing is manual.
Type of Change
src
or test files.Pre-Submission Checklist
npm run lint
).console.log
) has been removed.npm test
).main
branch.npm run changeset
if this PR includes user-facing changes or dependency updates.Screenshots / Videos
Before

After

Documentation Updates
Does this PR necessitate updates to user-facing documentation?
Important
Enhances mode selector UI with a new component, state management for new user interactions, and telemetry tracking in
TelemetryClient.ts
.ModeSelector
component inChatTextArea.tsx
to replaceSelectDropdown
for mode selection.modes.ts
for better user understanding.hasOpenedModeSelector
toglobalSettingsSchema
inglobal-settings.ts
andExtensionState
inExtensionMessage.ts
.ClineProvider.ts
andwebviewMessageHandler.ts
to managehasOpenedModeSelector
state.TelemetryClient.ts
to track mode selector interactions withsetPeopleProperty()
andgetPeopleProperty()
methods.This description was created by
for e45d2da. You can customize this summary. It will automatically update as commits are pushed.