From 18fdcde6ea57f84cd3922df7e255b320a6a962b4 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 26 Mar 2026 19:47:45 +0100 Subject: [PATCH] refactor(coding-agent): restructured validation and rendering for consistency - Refactored theme color validation to use single source of truth with THEME_COLOR_RECORD object. - Simplified model registry to defer per-model overrides to dedicated method and use constant for role IDs. - Refactored tree list rendering to pre-render items once for consistent line counts across phases. - Refactored question result formatting to use early returns and consistently include question ID in output. - Updated hook editor hint text to include ctrl+g external editor option when prompt style is enabled. - Removed unused isLogicalLineStart property from LayoutLine interface in editor component. --- packages/coding-agent/CHANGELOG.md | 8 +- .../coding-agent/src/config/model-registry.ts | 36 ++--- .../src/modes/components/hook-editor.ts | 2 +- .../coding-agent/src/modes/theme/theme.ts | 124 +++++++++--------- packages/coding-agent/src/tools/ask.ts | 13 +- packages/coding-agent/src/tui/tree-list.ts | 55 ++++---- packages/tui/src/components/editor.ts | 6 - 7 files changed, 115 insertions(+), 129 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index cf41c132b..0bd62b4e6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,11 +1,15 @@ # Changelog ## [Unreleased] - ### Added - Added `browser.screenshotDir` setting to configure screenshot save directory with path expansion +### Changed + +- Updated hook editor hint text to include `ctrl+g external editor` option when using prompt style +- Refactored question result formatting to consistently include question ID in output + ## [13.15.3] - 2026-03-26 ### Added @@ -6334,4 +6338,4 @@ Initial public release. - Git branch display in footer - Message queueing during streaming responses - OAuth integration for Gmail and Google Calendar access -- HTML export with syntax highlighting and collapsible sections +- HTML export with syntax highlighting and collapsible sections \ No newline at end of file diff --git a/packages/coding-agent/src/config/model-registry.ts b/packages/coding-agent/src/config/model-registry.ts index 09ec877ac..e17679a7f 100644 --- a/packages/coding-agent/src/config/model-registry.ts +++ b/packages/coding-agent/src/config/model-registry.ts @@ -69,9 +69,7 @@ export type RoleInfo = ModelRoleInfo; * entries across settings. */ export function getKnownRoleIds(settings: Settings): string[] { - // Avoid MODEL_ROLE_IDS here: this helper is reached during selector initialization, - // and model-registry participates in import cycles while the module is still evaluating. - const roles = ["default", "smol", "slow", "vision", "plan", "commit", "task"]; + const roles = [...MODEL_ROLE_IDS] as string[]; const seen = new Set(roles); const addRole = (role: string) => { if (seen.has(role)) return; @@ -832,7 +830,7 @@ export class ModelRegistry { this.#modelOverrides = modelOverrides; this.#addImplicitDiscoverableProviders(configuredProviders); - const builtInModels = this.#applyHardcodedModelPolicies(this.#loadBuiltInModels(overrides, modelOverrides)); + const builtInModels = this.#applyHardcodedModelPolicies(this.#loadBuiltInModels(overrides)); const cachedDiscoveries = this.#applyHardcodedModelPolicies(this.#loadCachedDiscoverableModels()); const resolvedDefaults = this.#mergeResolvedModels(builtInModels, cachedDiscoveries); const combined = this.#mergeCustomModels(resolvedDefaults, this.#customModelOverlays); @@ -840,31 +838,21 @@ export class ModelRegistry { this.#models = this.#applyModelOverrides(combined, this.#modelOverrides); } - /** Load built-in models, applying provider and per-model overrides */ - #loadBuiltInModels( - overrides: Map, - modelOverrides: Map>, - ): Model[] { + /** Load built-in models, applying provider-level overrides only. + * Per-model overrides are applied later by #applyModelOverrides. */ + #loadBuiltInModels(overrides: Map): Model[] { return getBundledProviders().flatMap(provider => { const models = getBundledModels(provider as Parameters[0]) as Model[]; const providerOverride = overrides.get(provider); - const perModelOverrides = modelOverrides.get(provider); return models.map(m => { - let model = m; - if (providerOverride) { - model = { - ...model, - baseUrl: providerOverride.baseUrl ?? model.baseUrl, - headers: providerOverride.headers ? { ...model.headers, ...providerOverride.headers } : model.headers, - compat: mergeCompat(model.compat, providerOverride.compat), - }; - } - const modelOverride = perModelOverrides?.get(m.id); - if (modelOverride) { - model = applyModelOverride(model, modelOverride); - } - return model; + if (!providerOverride) return m; + return { + ...m, + baseUrl: providerOverride.baseUrl ?? m.baseUrl, + headers: providerOverride.headers ? { ...m.headers, ...providerOverride.headers } : m.headers, + compat: mergeCompat(m.compat, providerOverride.compat), + }; }); }); } diff --git a/packages/coding-agent/src/modes/components/hook-editor.ts b/packages/coding-agent/src/modes/components/hook-editor.ts index da283f6ab..9abed7431 100644 --- a/packages/coding-agent/src/modes/components/hook-editor.ts +++ b/packages/coding-agent/src/modes/components/hook-editor.ts @@ -62,7 +62,7 @@ export class HookEditorComponent extends Container { // Hint const hint = this.#promptStyle - ? "enter submit esc cancel" + ? "enter submit esc cancel ctrl+g external editor" : "ctrl+enter submit esc cancel ctrl+g external editor"; this.addChild(new Text(theme.fg("dim", hint), 1, 0)); diff --git a/packages/coding-agent/src/modes/theme/theme.ts b/packages/coding-agent/src/modes/theme/theme.ts index 03157f4ab..1b411ddbd 100644 --- a/packages/coding-agent/src/modes/theme/theme.ts +++ b/packages/coding-agent/src/modes/theme/theme.ts @@ -956,67 +956,69 @@ export type ThemeColor = | "statusLineSubagents"; /** Set of all valid ThemeColor string values for runtime validation */ -const VALID_THEME_COLORS: ReadonlySet = new Set([ - "accent", - "border", - "borderAccent", - "borderMuted", - "success", - "error", - "warning", - "muted", - "dim", - "text", - "thinkingText", - "userMessageText", - "customMessageText", - "customMessageLabel", - "toolTitle", - "toolOutput", - "mdHeading", - "mdLink", - "mdLinkUrl", - "mdCode", - "mdCodeBlock", - "mdCodeBlockBorder", - "mdQuote", - "mdQuoteBorder", - "mdHr", - "mdListBullet", - "toolDiffAdded", - "toolDiffRemoved", - "toolDiffContext", - "syntaxComment", - "syntaxKeyword", - "syntaxFunction", - "syntaxVariable", - "syntaxString", - "syntaxNumber", - "syntaxType", - "syntaxOperator", - "syntaxPunctuation", - "thinkingOff", - "thinkingMinimal", - "thinkingLow", - "thinkingMedium", - "thinkingHigh", - "thinkingXhigh", - "bashMode", - "pythonMode", - "statusLineSep", - "statusLineModel", - "statusLinePath", - "statusLineGitClean", - "statusLineGitDirty", - "statusLineContext", - "statusLineSpend", - "statusLineStaged", - "statusLineDirty", - "statusLineUntracked", - "statusLineOutput", - "statusLineCost", - "statusLineSubagents", -]); +const THEME_COLOR_RECORD = { + accent: true, + border: true, + borderAccent: true, + borderMuted: true, + success: true, + error: true, + warning: true, + muted: true, + dim: true, + text: true, + thinkingText: true, + userMessageText: true, + customMessageText: true, + customMessageLabel: true, + toolTitle: true, + toolOutput: true, + mdHeading: true, + mdLink: true, + mdLinkUrl: true, + mdCode: true, + mdCodeBlock: true, + mdCodeBlockBorder: true, + mdQuote: true, + mdQuoteBorder: true, + mdHr: true, + mdListBullet: true, + toolDiffAdded: true, + toolDiffRemoved: true, + toolDiffContext: true, + syntaxComment: true, + syntaxKeyword: true, + syntaxFunction: true, + syntaxVariable: true, + syntaxString: true, + syntaxNumber: true, + syntaxType: true, + syntaxOperator: true, + syntaxPunctuation: true, + thinkingOff: true, + thinkingMinimal: true, + thinkingLow: true, + thinkingMedium: true, + thinkingHigh: true, + thinkingXhigh: true, + bashMode: true, + pythonMode: true, + statusLineSep: true, + statusLineModel: true, + statusLinePath: true, + statusLineGitClean: true, + statusLineGitDirty: true, + statusLineContext: true, + statusLineSpend: true, + statusLineStaged: true, + statusLineDirty: true, + statusLineUntracked: true, + statusLineOutput: true, + statusLineCost: true, + statusLineSubagents: true, +} satisfies Record; + +const VALID_THEME_COLORS: ReadonlySet = new Set(Object.keys(THEME_COLOR_RECORD)); /** Check if a string is a valid ThemeColor value */ export function isValidThemeColor(color: string): color is ThemeColor { diff --git a/packages/coding-agent/src/tools/ask.ts b/packages/coding-agent/src/tools/ask.ts index 13d4bb9eb..c353a0e65 100644 --- a/packages/coding-agent/src/tools/ask.ts +++ b/packages/coding-agent/src/tools/ask.ts @@ -354,14 +354,15 @@ async function askSingleQuestion( } function formatQuestionResult(result: QuestionResult): string { - const parts: string[] = []; - if (result.selectedOptions.length > 0) { - parts.push(result.multi ? `[${result.selectedOptions.join(", ")}]` : result.selectedOptions[0]!); - } if (result.customInput !== undefined) { - parts.push(`"${result.customInput}"`); + return `${result.id}: "${result.customInput}"`; } - return parts.length > 0 ? `${result.id}: ${parts.join(", ")}` : `${result.id}: (cancelled)`; + if (result.selectedOptions.length > 0) { + return result.multi + ? `${result.id}: [${result.selectedOptions.join(", ")}]` + : `${result.id}: ${result.selectedOptions[0]}`; + } + return `${result.id}: (cancelled)`; } // ============================================================================= diff --git a/packages/coding-agent/src/tui/tree-list.ts b/packages/coding-agent/src/tui/tree-list.ts index 3c2abf1af..a4fbc582d 100644 --- a/packages/coding-agent/src/tui/tree-list.ts +++ b/packages/coding-agent/src/tui/tree-list.ts @@ -15,6 +15,8 @@ export interface TreeListOptions { */ maxCollapsedLines?: number; itemType?: string; + /** Called once per item with `isLast: false` during budget calculation; + * line count MUST NOT vary based on `isLast`. */ renderItem: (item: T, context: TreeContext) => string | string[]; } @@ -23,22 +25,29 @@ export function renderTreeList(options: TreeListOptions, theme: Theme): st const maxItems = expanded ? items.length : Math.min(items.length, maxCollapsed); const linesBudget = !expanded && maxCollapsedLines !== undefined ? maxCollapsedLines : Infinity; - // Pass 1: determine how many items fit within both the item count and total line budget, - // including the trailing summary row when one is needed. + // Pre-render each candidate item once. + // isLast cannot be known at this point (fittingCount is not yet determined); + // renderItem implementations MUST NOT vary line count based on isLast. + const preRendered: string[][] = []; + for (let i = 0; i < maxItems; i++) { + const rendered = renderItem(items[i], { + index: i, + isLast: false, + depth: 0, + theme, + prefix: "", + continuePrefix: "", + }); + preRendered.push(Array.isArray(rendered) ? rendered : rendered ? [rendered] : []); + } + + // Determine how many items fit within the line budget. let fittingCount = maxItems; let fittedLineCount = 0; if (linesBudget !== Infinity) { fittingCount = 0; for (let i = 0; i < maxItems; i++) { - const rendered = renderItem(items[i], { - index: i, - isLast: false, - depth: 0, - theme, - prefix: "", - continuePrefix: "", - }); - const count = Array.isArray(rendered) ? rendered.length : rendered ? 1 : 0; + const count = preRendered[i]!.length; const remainingAfter = items.length - (i + 1); const reservedSummaryLines = remainingAfter > 0 ? 1 : 0; if (fittedLineCount + count + reservedSummaryLines > linesBudget) break; @@ -50,30 +59,18 @@ export function renderTreeList(options: TreeListOptions, theme: Theme): st const remaining = items.length - fittingCount; const hasSummary = !expanded && remaining > 0 && (linesBudget === Infinity || fittedLineCount < linesBudget); - // Pass 2: render items with correct isLast and prefixes. + // Emit pre-rendered content with correct isLast-based branch prefixes. const lines: string[] = []; for (let i = 0; i < fittingCount; i++) { const isLast = !hasSummary && i === fittingCount - 1; const branch = getTreeBranch(isLast, theme); const prefix = `${theme.fg("dim", branch)} `; const continuePrefix = `${theme.fg("dim", getTreeContinuePrefix(isLast, theme))}`; - const context: TreeContext = { - index: i, - isLast, - depth: 0, - theme, - prefix, - continuePrefix, - }; - const rendered = renderItem(items[i], context); - if (Array.isArray(rendered)) { - if (rendered.length === 0) continue; - lines.push(`${prefix}${replaceTabs(rendered[0])}`); - for (let j = 1; j < rendered.length; j++) { - lines.push(`${continuePrefix}${replaceTabs(rendered[j])}`); - } - } else { - lines.push(`${prefix}${replaceTabs(rendered)}`); + const itemLines = preRendered[i]!; + if (itemLines.length === 0) continue; + lines.push(`${prefix}${replaceTabs(itemLines[0]!)}`); + for (let j = 1; j < itemLines.length; j++) { + lines.push(`${continuePrefix}${replaceTabs(itemLines[j]!)}`); } } diff --git a/packages/tui/src/components/editor.ts b/packages/tui/src/components/editor.ts index 4e006c5eb..522e4a6b7 100644 --- a/packages/tui/src/components/editor.ts +++ b/packages/tui/src/components/editor.ts @@ -281,7 +281,6 @@ interface LayoutLine { text: string; hasCursor: boolean; cursorPos?: number; - isLogicalLineStart: boolean; } export interface EditorTheme { @@ -1232,7 +1231,6 @@ export class Editor implements Component, Focusable { text: "", hasCursor: true, cursorPos: 0, - isLogicalLineStart: true, }); return layoutLines; } @@ -1250,13 +1248,11 @@ export class Editor implements Component, Focusable { text: line, hasCursor: true, cursorPos: this.#state.cursorCol, - isLogicalLineStart: true, }); } else { layoutLines.push({ text: line, hasCursor: false, - isLogicalLineStart: true, }); } } else { @@ -1300,13 +1296,11 @@ export class Editor implements Component, Focusable { text: chunk.text, hasCursor: true, cursorPos: adjustedCursorPos, - isLogicalLineStart: chunkIndex === 0, }); } else { layoutLines.push({ text: chunk.text, hasCursor: false, - isLogicalLineStart: chunkIndex === 0, }); } }