fix: address PR #5582 review feedback

- Standardize default max diagnostic messages to 50 across all files
- Extract diagnostic settings to centralized constants and helper functions
- Add comprehensive documentation for diagnostic settings
- Update UI to always show max diagnostic messages slider
- Add unit tests for ContextManagementSettings component
- Improve code organization and maintainability

Author: hannesrudolph

packages/types/src/global-settings.ts

@@ -72,7 +72,15 @@ export const globalSettingsSchema = z.object({
 	autoCondenseContextPercent: z.number().optional(),
 	maxConcurrentFileReads: z.number().optional(),
 
+	/**
+	 * Whether to include diagnostic messages (errors, warnings) in tool outputs
+	 * @default true
+	 */
 	includeDiagnosticMessages: z.boolean().optional(),
+	/**
+	 * Maximum number of diagnostic messages to include in tool outputs
+	 * @default 50
+	 */
 	maxDiagnosticMessages: z.number().optional(),
 
 	browserToolEnabled: z.boolean().optional(),

src/core/constants/diagnosticSettings.ts

@@ -0,0 +1,13 @@
+/**
+ * Constants for diagnostic settings
+ */
+
+/**
+ * Default value for whether to include diagnostic messages in tool outputs
+ */
+export const DEFAULT_INCLUDE_DIAGNOSTIC_MESSAGES = true
+
+/**
+ * Default maximum number of diagnostic messages to include in tool outputs
+ */
+export const DEFAULT_MAX_DIAGNOSTIC_MESSAGES = 50

src/core/tools/applyDiffTool.ts

@@ -12,6 +12,7 @@ import { formatResponse } from "../prompts/responses"
 import { fileExistsAtPath } from "../../utils/fs"
 import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
 import { unescapeHtmlEntities } from "../../utils/text-normalization"
+import { getDiagnosticSettings } from "./helpers/diagnosticSettings"
 
 export async function applyDiffToolLegacy(
 	cline: Task,
@@ -142,10 +143,8 @@ export async function applyDiffToolLegacy(
 			cline.consecutiveMistakeCount = 0
 			cline.consecutiveMistakeCountForApplyDiff.delete(relPath)
 
-			// Get diagnostic settings from state
-			const state = await cline.providerRef?.deref()?.getState()
-			const includeDiagnosticMessages = state?.includeDiagnosticMessages ?? true
-			const maxDiagnosticMessages = state?.maxDiagnosticMessages
+			// Get diagnostic settings
+			const { includeDiagnosticMessages, maxDiagnosticMessages } = await getDiagnosticSettings(cline)
 
 			// Update DiffViewProvider with diagnostic settings
 			cline.diffViewProvider.updateDiagnosticSettings(includeDiagnosticMessages, maxDiagnosticMessages)

src/core/tools/helpers/diagnosticSettings.ts

@@ -0,0 +1,22 @@
+import { Task } from "../../task/Task"
+import {
+	DEFAULT_INCLUDE_DIAGNOSTIC_MESSAGES,
+	DEFAULT_MAX_DIAGNOSTIC_MESSAGES,
+} from "../../constants/diagnosticSettings"
+
+/**
+ * Retrieves diagnostic settings from the provider state
+ * @param cline - The Task instance
+ * @returns Object containing diagnostic settings with defaults
+ */
+export async function getDiagnosticSettings(cline: Task): Promise<{
+	includeDiagnosticMessages: boolean
+	maxDiagnosticMessages: number
+}> {
+	const state = await cline.providerRef?.deref()?.getState()
+
+	return {
+		includeDiagnosticMessages: state?.includeDiagnosticMessages ?? DEFAULT_INCLUDE_DIAGNOSTIC_MESSAGES,
+		maxDiagnosticMessages: state?.maxDiagnosticMessages ?? DEFAULT_MAX_DIAGNOSTIC_MESSAGES,
+	}
+}

src/core/tools/insertContentTool.ts

@@ -11,6 +11,7 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
 import { fileExistsAtPath } from "../../utils/fs"
 import { insertGroups } from "../diff/insert-groups"
 import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
+import { getDiagnosticSettings } from "./helpers/diagnosticSettings"
 
 export async function insertContentTool(
 	cline: Task,
@@ -99,14 +100,11 @@ export async function insertContentTool(
 		cline.diffViewProvider.editType = fileExists ? "modify" : "create"
 		cline.diffViewProvider.originalContent = fileContent
 
-		// Update diagnostic settings from global state
-		const state = await cline.providerRef?.deref()?.getState()
-		if (state) {
-			cline.diffViewProvider.updateDiagnosticSettings(
-				state.includeDiagnosticMessages ?? true,
-				state.maxDiagnosticMessages ?? 5,
-			)
-		}
+		// Get diagnostic settings
+		const { includeDiagnosticMessages, maxDiagnosticMessages } = await getDiagnosticSettings(cline)
+
+		// Update DiffViewProvider with diagnostic settings
+		cline.diffViewProvider.updateDiagnosticSettings(includeDiagnosticMessages, maxDiagnosticMessages)
 		const lines = fileExists ? fileContent.split("\n") : []
 
 		const updatedContent = insertGroups(lines, [

src/core/tools/searchAndReplaceTool.ts

@@ -12,6 +12,7 @@ import { getReadablePath } from "../../utils/path"
 import { fileExistsAtPath } from "../../utils/fs"
 import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
 import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
+import { getDiagnosticSettings } from "./helpers/diagnosticSettings"
 
 /**
  * Tool for performing search and replace operations on files
@@ -191,14 +192,11 @@ export async function searchAndReplaceTool(
 		cline.diffViewProvider.editType = "modify"
 		cline.diffViewProvider.originalContent = fileContent
 
-		// Update diagnostic settings from global state
-		const state = await cline.providerRef?.deref()?.getState()
-		if (state) {
-			cline.diffViewProvider.updateDiagnosticSettings(
-				state.includeDiagnosticMessages ?? true,
-				state.maxDiagnosticMessages ?? 5,
-			)
-		}
+		// Get diagnostic settings
+		const { includeDiagnosticMessages, maxDiagnosticMessages } = await getDiagnosticSettings(cline)
+
+		// Update DiffViewProvider with diagnostic settings
+		cline.diffViewProvider.updateDiagnosticSettings(includeDiagnosticMessages, maxDiagnosticMessages)
 
 		// Generate and validate diff
 		const diff = formatResponse.createPrettyPatch(validRelPath, fileContent, newContent)

src/core/tools/writeToFileTool.ts

@@ -14,6 +14,7 @@ import { isPathOutsideWorkspace } from "../../utils/pathUtils"
 import { detectCodeOmission } from "../../integrations/editor/detect-omission"
 import { unescapeHtmlEntities } from "../../utils/text-normalization"
 import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
+import { getDiagnosticSettings } from "./helpers/diagnosticSettings"
 
 export async function writeToFileTool(
 	cline: Task,
@@ -97,10 +98,8 @@ export async function writeToFileTool(
 		isProtected: isWriteProtected,
 	}
 
-	// Get diagnostic settings from state
-	const state = await cline.providerRef?.deref()?.getState()
-	const includeDiagnosticMessages = state?.includeDiagnosticMessages ?? true
-	const maxDiagnosticMessages = state?.maxDiagnosticMessages
+	// Get diagnostic settings
+	const { includeDiagnosticMessages, maxDiagnosticMessages } = await getDiagnosticSettings(cline)
 
 	// Update DiffViewProvider with diagnostic settings
 	cline.diffViewProvider.updateDiagnosticSettings(includeDiagnosticMessages, maxDiagnosticMessages)

src/core/webview/ClineProvider.ts

@@ -71,6 +71,7 @@ import { WebviewMessage } from "../../shared/WebviewMessage"
 import { EMBEDDING_MODEL_PROFILES } from "../../shared/embeddingModels"
 import { ProfileValidator } from "../../shared/ProfileValidator"
 import { getWorkspaceGitInfo } from "../../utils/git"
+import { DEFAULT_INCLUDE_DIAGNOSTIC_MESSAGES, DEFAULT_MAX_DIAGNOSTIC_MESSAGES } from "../constants/diagnosticSettings"
 
 /**
  * https://github.com/microsoft/vscode-webview-ui-toolkit-samples/blob/main/default/weather-webview/src/providers/WeatherViewProvider.ts
@@ -1727,8 +1728,8 @@ export class ClineProvider
 			},
 			profileThresholds: stateValues.profileThresholds ?? {},
 			// Add diagnostic message settings
-			includeDiagnosticMessages: stateValues.includeDiagnosticMessages ?? true,
-			maxDiagnosticMessages: stateValues.maxDiagnosticMessages ?? 5,
+			includeDiagnosticMessages: stateValues.includeDiagnosticMessages ?? DEFAULT_INCLUDE_DIAGNOSTIC_MESSAGES,
+			maxDiagnosticMessages: stateValues.maxDiagnosticMessages ?? DEFAULT_MAX_DIAGNOSTIC_MESSAGES,
 		}
 	}
 

webview-ui/src/components/settings/ContextManagementSettings.tsx

@@ -217,27 +217,25 @@ export const ContextManagementSettings = ({
 					</div>
 				</div>
 
-				{includeDiagnosticMessages && (
-					<div>
-						<span className="block font-medium mb-1">
-							{t("settings:contextManagement.diagnostics.maxMessages.label")}
-						</span>
-						<div className="flex items-center gap-2">
-							<Slider
-								min={1}
-								max={100}
-								step={1}
-								value={[maxDiagnosticMessages ?? 50]}
-								onValueChange={([value]) => setCachedStateField("maxDiagnosticMessages", value)}
-								data-testid="max-diagnostic-messages-slider"
-							/>
-							<span className="w-10">{maxDiagnosticMessages ?? 50}</span>
-						</div>
-						<div className="text-vscode-descriptionForeground text-sm mt-1">
-							{t("settings:contextManagement.diagnostics.maxMessages.description")}
-						</div>
+				<div>
+					<span className="block font-medium mb-1">
+						{t("settings:contextManagement.diagnostics.maxMessages.label")}
+					</span>
+					<div className="flex items-center gap-2">
+						<Slider
+							min={1}
+							max={100}
+							step={1}
+							value={[maxDiagnosticMessages ?? 50]}
+							onValueChange={([value]) => setCachedStateField("maxDiagnosticMessages", value)}
+							data-testid="max-diagnostic-messages-slider"
+						/>
+						<span className="w-10">{maxDiagnosticMessages ?? 50}</span>
 					</div>
-				)}
+					<div className="text-vscode-descriptionForeground text-sm mt-1">
+						{t("settings:contextManagement.diagnostics.maxMessages.description")}
+					</div>
+				</div>
 			</Section>
 			<Section className="pt-2">
 				<VSCodeCheckbox

webview-ui/src/components/settings/__tests__/ContextManagementSettings.spec.tsx

@@ -9,7 +9,7 @@ vi.mock("@/components/ui", () => ({
 	Slider: ({ value, onValueChange, "data-testid": dataTestId, disabled }: any) => (
 		<input
 			type="range"
-			value={value[0]}
+			value={value?.[0] ?? 0}
 			onChange={(e) => onValueChange([parseFloat(e.target.value)])}
 			data-testid={dataTestId}
 			disabled={disabled}
@@ -106,8 +106,9 @@ describe("ContextManagementSettings", () => {
 		const checkbox = screen.getByTestId("include-diagnostic-messages-checkbox")
 		expect(checkbox.querySelector("input")).not.toBeChecked()
 
-		// Slider should not be rendered when diagnostics are disabled
-		expect(screen.queryByTestId("max-diagnostic-messages-slider")).not.toBeInTheDocument()
+		// Slider should still be rendered when diagnostics are disabled
+		expect(screen.getByTestId("max-diagnostic-messages-slider")).toBeInTheDocument()
+		expect(screen.getByText("50")).toBeInTheDocument()
 	})
 
 	it("calls setCachedStateField when include diagnostic messages checkbox is toggled", async () => {
@@ -134,15 +135,15 @@ describe("ContextManagementSettings", () => {
 		})
 	})
 
-	it("hides slider when include diagnostic messages is unchecked", () => {
+	it("keeps slider visible when include diagnostic messages is unchecked", () => {
 		const { rerender } = render(<ContextManagementSettings {...defaultProps} includeDiagnosticMessages={true} />)
 
 		const slider = screen.getByTestId("max-diagnostic-messages-slider")
 		expect(slider).toBeInTheDocument()
 
-		// Update to disabled
+		// Update to disabled - slider should still be visible
 		rerender(<ContextManagementSettings {...defaultProps} includeDiagnosticMessages={false} />)
-		expect(screen.queryByTestId("max-diagnostic-messages-slider")).not.toBeInTheDocument()
+		expect(screen.getByTestId("max-diagnostic-messages-slider")).toBeInTheDocument()
 	})
 
 	it("displays correct max diagnostic messages value", () => {
@@ -167,4 +168,110 @@ describe("ContextManagementSettings", () => {
 		expect(screen.getByTestId("show-rooignored-files-checkbox")).toBeInTheDocument()
 		expect(screen.getByTestId("auto-condense-context-checkbox")).toBeInTheDocument()
 	})
+
+	describe("Edge cases for maxDiagnosticMessages", () => {
+		it("handles zero value correctly", async () => {
+			const setCachedStateField = vi.fn()
+			render(
+				<ContextManagementSettings
+					{...defaultProps}
+					maxDiagnosticMessages={0}
+					setCachedStateField={setCachedStateField}
+				/>,
+			)
+
+			expect(screen.getByText("0")).toBeInTheDocument()
+
+			const slider = screen.getByTestId("max-diagnostic-messages-slider")
+			expect(slider).toHaveValue("0")
+		})
+
+		it("handles negative values by displaying them", async () => {
+			const setCachedStateField = vi.fn()
+			render(
+				<ContextManagementSettings
+					{...defaultProps}
+					maxDiagnosticMessages={-10}
+					setCachedStateField={setCachedStateField}
+				/>,
+			)
+
+			// Component displays the actual negative value in the text span
+			expect(screen.getByText("-10")).toBeInTheDocument()
+
+			// Note: The actual slider behavior with negative values depends on the implementation
+			// In this case, we're just verifying the component renders without errors
+		})
+
+		it("handles very large numbers by capping at maximum", async () => {
+			const setCachedStateField = vi.fn()
+			const largeNumber = 1000
+			render(
+				<ContextManagementSettings
+					{...defaultProps}
+					maxDiagnosticMessages={largeNumber}
+					setCachedStateField={setCachedStateField}
+				/>,
+			)
+
+			// Should display the actual value even if it exceeds slider max
+			expect(screen.getByText(largeNumber.toString())).toBeInTheDocument()
+
+			// Slider value would be capped at max (100)
+			const slider = screen.getByTestId("max-diagnostic-messages-slider")
+			expect(slider).toHaveValue("100")
+		})
+
+		it("enforces maximum value constraint", async () => {
+			const setCachedStateField = vi.fn()
+			render(<ContextManagementSettings {...defaultProps} setCachedStateField={setCachedStateField} />)
+
+			const slider = screen.getByTestId("max-diagnostic-messages-slider")
+
+			// Test that setting value above 100 gets capped
+			fireEvent.change(slider, { target: { value: "150" } })
+
+			await waitFor(() => {
+				// Should be capped at 100 (the slider's max)
+				expect(setCachedStateField).toHaveBeenCalledWith("maxDiagnosticMessages", 100)
+			})
+		})
+
+		it("handles boundary value at minimum (0)", async () => {
+			const setCachedStateField = vi.fn()
+			render(<ContextManagementSettings {...defaultProps} setCachedStateField={setCachedStateField} />)
+
+			const slider = screen.getByTestId("max-diagnostic-messages-slider")
+			fireEvent.change(slider, { target: { value: "0" } })
+
+			await waitFor(() => {
+				expect(setCachedStateField).toHaveBeenCalledWith("maxDiagnosticMessages", 0)
+			})
+		})
+
+		it("handles boundary value at maximum (100)", async () => {
+			const setCachedStateField = vi.fn()
+			render(<ContextManagementSettings {...defaultProps} setCachedStateField={setCachedStateField} />)
+
+			const slider = screen.getByTestId("max-diagnostic-messages-slider")
+			fireEvent.change(slider, { target: { value: "100" } })
+
+			await waitFor(() => {
+				expect(setCachedStateField).toHaveBeenCalledWith("maxDiagnosticMessages", 100)
+			})
+		})
+
+		it("handles decimal values by parsing as float", async () => {
+			const setCachedStateField = vi.fn()
+			render(<ContextManagementSettings {...defaultProps} setCachedStateField={setCachedStateField} />)
+
+			const slider = screen.getByTestId("max-diagnostic-messages-slider")
+			fireEvent.change(slider, { target: { value: "50.7" } })
+
+			await waitFor(() => {
+				// The mock slider component parses as float
+				expect(setCachedStateField).toHaveBeenCalledWith("maxDiagnosticMessages", 50.7)
+			})
+		})
+	})
 })