Skip to content

fix: improve save button activation in prompts settings (#5780) #8267

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 3, 2025
Merged

fix: improve save button activation in prompts settings (#5780) #8267

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f913f9c
fix: resolve save button activation in prompts settings (#5780)
MuriloFP Jul 21, 2025
7eae039
fix: ensure change detection triggers for all prompt types
MuriloFP Jul 21, 2025
358a76b
fix: remove immediate trim in onChange to ensure proper state updates
MuriloFP Jul 21, 2025
61aed25
fix: improve save button activation in prompts settings
roomote Sep 23, 2025
93ea9e0
test: update test expectations for nullish coalescing behavior
roomote Sep 23, 2025
bf3f2ff
fix(webview): preserve empty strings using nullish coalescing in prom…
roomote Sep 24, 2025
cc26220
chore: remove stray nested gitlink tmp/Roo-Code
roomote Sep 24, 2025
cae2337
fix(settings/prompts): enable Save on textarea input and preserve emp…
daniel-lxs Sep 25, 2025
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
14 changes: 7 additions & 7 deletions webview-ui/src/components/modes/ModesView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,7 @@ const ModesView = ({ onDone }: ModesViewProps) => {
})()}
onChange={(e) => {
const value =
(e as unknown as CustomEvent)?.detail?.target?.value ||
(e as unknown as CustomEvent)?.detail?.target?.value ??
((e as any).target as HTMLTextAreaElement).value
const customMode = findModeBySlug(visualMode, customModes)
if (customMode) {
Expand Down Expand Up @@ -888,7 +888,7 @@ const ModesView = ({ onDone }: ModesViewProps) => {
})()}
onChange={(e) => {
const value =
(e as unknown as CustomEvent)?.detail?.target?.value ||
(e as unknown as CustomEvent)?.detail?.target?.value ??
((e as any).target as HTMLTextAreaElement).value
const customMode = findModeBySlug(visualMode, customModes)
if (customMode) {
Expand Down Expand Up @@ -943,7 +943,7 @@ const ModesView = ({ onDone }: ModesViewProps) => {
})()}
onChange={(e) => {
const value =
(e as unknown as CustomEvent)?.detail?.target?.value ||
(e as unknown as CustomEvent)?.detail?.target?.value ??
((e as any).target as HTMLTextAreaElement).value
const customMode = findModeBySlug(visualMode, customModes)
if (customMode) {
Expand Down Expand Up @@ -1102,7 +1102,7 @@ const ModesView = ({ onDone }: ModesViewProps) => {
})()}
onChange={(e) => {
const value =
(e as unknown as CustomEvent)?.detail?.target?.value ||
(e as unknown as CustomEvent)?.detail?.target?.value ??
((e as any).target as HTMLTextAreaElement).value
const customMode = findModeBySlug(visualMode, customModes)
if (customMode) {
Expand Down Expand Up @@ -1307,12 +1307,12 @@ const ModesView = ({ onDone }: ModesViewProps) => {
value={customInstructions || ""}
onChange={(e) => {
const value =
(e as unknown as CustomEvent)?.detail?.target?.value ||
(e as unknown as CustomEvent)?.detail?.target?.value ??
((e as any).target as HTMLTextAreaElement).value
setCustomInstructions(value || undefined)
setCustomInstructions(value ?? undefined)
vscode.postMessage({
type: "customInstructions",
text: value.trim() || undefined,
text: value.trim() ?? undefined,
Copy link
Member

@daniel-lxs daniel-lxs Sep 24, 2025

Choose a reason for hiding this comment

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

P2: This still trims during onChange, which conflicts with preserving whitespace while editing. Recommend posting the raw value here and trimming only on save to avoid state/message mismatches.

P3: Minor: ?? undefined after trim() is redundant since trim() always returns a string.

})
}}
rows={4}
Expand Down
9 changes: 5 additions & 4 deletions webview-ui/src/components/modes/__tests__/ModesView.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,13 @@ describe("PromptsView", () => {
const changeEvent = new Event("change", { bubbles: true })
fireEvent(textarea, changeEvent)

// The component calls setCustomInstructions with value || undefined
// Since empty string is falsy, it should be undefined
expect(setCustomInstructions).toHaveBeenCalledWith(undefined)
// The component calls setCustomInstructions with value ?? undefined
// With nullish coalescing, empty string is preserved (not treated as nullish)
expect(setCustomInstructions).toHaveBeenCalledWith("")
Copy link
Contributor Author

@roomote roomote bot Sep 23, 2025

Choose a reason for hiding this comment

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

Test Coverage Gap: While the test correctly verifies the onChange behavior with nullish coalescing, it doesn't test the actual save functionality or the trimming that should happen on save. Consider adding tests for the complete save flow.

// The postMessage call will have multiple calls, we need to check the right one
expect(vscode.postMessage).toHaveBeenCalledWith({
type: "customInstructions",
text: undefined,
text: "", // empty string is now preserved with ?? operator
})
})
})
32 changes: 26 additions & 6 deletions webview-ui/src/components/settings/PromptsSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,31 @@ const PromptsSettings = ({ customSupportPrompts, setCustomSupportPrompts }: Prom
}, [])

const updateSupportPrompt = (type: SupportPromptType, value: string | undefined) => {
// Don't trim during editing to preserve intentional whitespace
// Use nullish coalescing to preserve empty strings
const finalValue = value ?? undefined
Copy link
Contributor Author

@roomote roomote bot Sep 23, 2025

Choose a reason for hiding this comment

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

Suggestion: Using value ?? undefined is redundant since the nullish coalescing operator already handles null/undefined. This should just be const finalValue = value.


if (type === "CONDENSE") {
setCustomCondensingPrompt(value || supportPrompt.default.CONDENSE)
setCustomCondensingPrompt(finalValue ?? supportPrompt.default.CONDENSE)
vscode.postMessage({
type: "updateCondensingPrompt",
text: value || supportPrompt.default.CONDENSE,
text: finalValue ?? supportPrompt.default.CONDENSE,
})
// Also update the customSupportPrompts to trigger change detection
const updatedPrompts = { ...customSupportPrompts }
if (finalValue === undefined) {
delete updatedPrompts[type]
} else {
updatedPrompts[type] = finalValue
}
setCustomSupportPrompts(updatedPrompts)
Copy link
Contributor Author

@roomote roomote bot Sep 23, 2025

Choose a reason for hiding this comment

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

Critical Issue: This state update for CONDENSE type is redundant. The parent component already manages customSupportPrompts state through the setCustomSupportPrompts prop. This duplicate state management could cause synchronization issues and unnecessary re-renders.

Consider removing lines 69-76 since the state is already being updated properly through the parent's setter function.

} else {
const updatedPrompts = { ...customSupportPrompts, [type]: value }
const updatedPrompts = { ...customSupportPrompts }
if (finalValue === undefined) {
delete updatedPrompts[type]
} else {
updatedPrompts[type] = finalValue
}
Copy link
Contributor Author

@roomote roomote bot Sep 23, 2025

Choose a reason for hiding this comment

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

Minor: This pattern of updating customSupportPrompts by either deleting or setting keys is repeated multiple times (lines 71-75, 79-83, 96-98, 100-102). Consider extracting this into a helper function:

const updatePromptsObject = (prompts: Record<string, string | undefined>, key: string, value: string | undefined) => {
  const updated = { ...prompts };
  if (value === undefined) {
    delete updated[key];
  } else {
    updated[key] = value;
  }
  return updated;
}

setCustomSupportPrompts(updatedPrompts)
}
}
Expand All @@ -75,6 +92,10 @@ const PromptsSettings = ({ customSupportPrompts, setCustomSupportPrompts }: Prom
type: "updateCondensingPrompt",
text: supportPrompt.default.CONDENSE,
})
// Also update the customSupportPrompts to trigger change detection
const updatedPrompts = { ...customSupportPrompts }
delete updatedPrompts[type]
setCustomSupportPrompts(updatedPrompts)
} else {
const updatedPrompts = { ...customSupportPrompts }
delete updatedPrompts[type]
Expand Down Expand Up @@ -147,10 +168,9 @@ const PromptsSettings = ({ customSupportPrompts, setCustomSupportPrompts }: Prom
value={getSupportPromptValue(activeSupportOption)}
onChange={(e) => {
const value =
(e as unknown as CustomEvent)?.detail?.target?.value ||
(e as unknown as CustomEvent)?.detail?.target?.value ??
((e as any).target as HTMLTextAreaElement).value
const trimmedValue = value.trim()
updateSupportPrompt(activeSupportOption, trimmedValue || undefined)
updateSupportPrompt(activeSupportOption, value)
Copy link
Contributor Author

@roomote roomote bot Sep 23, 2025

Choose a reason for hiding this comment

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

Critical Issue: The PR description mentions "Trimming only happens on save, not on every keystroke", but I don't see any trimming logic when the Save button is actually clicked. The values should be trimmed before being persisted to storage.

Consider adding a save handler that trims values before persisting them.

}}
rows={6}
className="w-full"
Expand Down
Loading