From f007ca2c8001a0083e6208ee8262fe0e1f149822 Mon Sep 17 00:00:00 2001 From: Stefan de Vogelaere Date: Tue, 20 Jan 2026 20:16:25 +0100 Subject: [PATCH] refactor: simplify getPhaseModelWithOverrides calls per code review Address code review feedback on PR #629: - Make settingsService parameter optional in getPhaseModelWithOverrides - Function now handles undefined settingsService gracefully by returning defaults - Remove redundant ternary checks in 4 call sites: - apps/server/src/routes/context/routes/describe-file.ts - apps/server/src/routes/context/routes/describe-image.ts - apps/server/src/routes/worktree/routes/generate-commit-message.ts - apps/server/src/services/auto-mode-service.ts - Remove unused DEFAULT_PHASE_MODELS imports where applicable --- apps/server/src/lib/settings-helpers.ts | 16 +++++++++++++-- .../routes/context/routes/describe-file.ts | 19 ++++++------------ .../routes/context/routes/describe-image.ts | 20 +++++++------------ .../routes/generate-commit-message.ts | 20 +++++++------------ apps/server/src/services/auto-mode-service.ts | 18 ++++++----------- 5 files changed, 40 insertions(+), 53 deletions(-) diff --git a/apps/server/src/lib/settings-helpers.ts b/apps/server/src/lib/settings-helpers.ts index 9ecde776..f5a66c18 100644 --- a/apps/server/src/lib/settings-helpers.ts +++ b/apps/server/src/lib/settings-helpers.ts @@ -15,6 +15,7 @@ import type { PhaseModelEntry, Credentials, } from '@automaker/types'; +import { DEFAULT_PHASE_MODELS } from '@automaker/types'; import { mergeAutoModePrompts, mergeAgentPrompts, @@ -502,17 +503,28 @@ export interface PhaseModelWithOverridesResult { * Also resolves the provider if the phase model has a providerId. * * @param phase - The phase key (e.g., 'enhancementModel', 'specGenerationModel') - * @param settingsService - Settings service instance + * @param settingsService - Optional settings service instance (returns defaults if undefined) * @param projectPath - Optional project path for checking overrides * @param logPrefix - Prefix for log messages * @returns Promise resolving to phase model with provider info */ export async function getPhaseModelWithOverrides( phase: PhaseModelKey, - settingsService: SettingsService, + settingsService?: SettingsService | null, projectPath?: string, logPrefix = '[SettingsHelper]' ): Promise { + // Handle undefined settingsService gracefully + if (!settingsService) { + logger.info(`${logPrefix} SettingsService not available, using default for ${phase}`); + return { + phaseModel: DEFAULT_PHASE_MODELS[phase] || { model: 'sonnet' }, + isProjectOverride: false, + provider: undefined, + credentials: undefined, + }; + } + try { const globalSettings = await settingsService.getGlobalSettings(); const credentials = await settingsService.getCredentials(); diff --git a/apps/server/src/routes/context/routes/describe-file.ts b/apps/server/src/routes/context/routes/describe-file.ts index 2742530b..a59dfb74 100644 --- a/apps/server/src/routes/context/routes/describe-file.ts +++ b/apps/server/src/routes/context/routes/describe-file.ts @@ -12,7 +12,6 @@ import type { Request, Response } from 'express'; import { createLogger } from '@automaker/utils'; -import { DEFAULT_PHASE_MODELS } from '@automaker/types'; import { PathNotAllowedError } from '@automaker/platform'; import { resolvePhaseModel } from '@automaker/model-resolver'; import { simpleQuery } from '../../../providers/simple-query-service.js'; @@ -161,18 +160,12 @@ ${contentToAnalyze}`; phaseModel: phaseModelEntry, provider, credentials, - } = settingsService - ? await getPhaseModelWithOverrides( - 'fileDescriptionModel', - settingsService, - cwd, - '[DescribeFile]' - ) - : { - phaseModel: DEFAULT_PHASE_MODELS.fileDescriptionModel, - provider: undefined, - credentials: undefined, - }; + } = await getPhaseModelWithOverrides( + 'fileDescriptionModel', + settingsService, + cwd, + '[DescribeFile]' + ); const { model, thinkingLevel } = resolvePhaseModel(phaseModelEntry); logger.info( diff --git a/apps/server/src/routes/context/routes/describe-image.ts b/apps/server/src/routes/context/routes/describe-image.ts index 81a4cd09..018a932c 100644 --- a/apps/server/src/routes/context/routes/describe-image.ts +++ b/apps/server/src/routes/context/routes/describe-image.ts @@ -13,7 +13,7 @@ import type { Request, Response } from 'express'; import { createLogger, readImageAsBase64 } from '@automaker/utils'; -import { DEFAULT_PHASE_MODELS, isCursorModel } from '@automaker/types'; +import { isCursorModel } from '@automaker/types'; import { resolvePhaseModel } from '@automaker/model-resolver'; import { simpleQuery } from '../../../providers/simple-query-service.js'; import * as secureFs from '../../../lib/secure-fs.js'; @@ -279,18 +279,12 @@ export function createDescribeImageHandler( phaseModel: phaseModelEntry, provider, credentials, - } = settingsService - ? await getPhaseModelWithOverrides( - 'imageDescriptionModel', - settingsService, - cwd, - '[DescribeImage]' - ) - : { - phaseModel: DEFAULT_PHASE_MODELS.imageDescriptionModel, - provider: undefined, - credentials: undefined, - }; + } = await getPhaseModelWithOverrides( + 'imageDescriptionModel', + settingsService, + cwd, + '[DescribeImage]' + ); const { model, thinkingLevel } = resolvePhaseModel(phaseModelEntry); logger.info( diff --git a/apps/server/src/routes/worktree/routes/generate-commit-message.ts b/apps/server/src/routes/worktree/routes/generate-commit-message.ts index 68ae8cc4..9335204f 100644 --- a/apps/server/src/routes/worktree/routes/generate-commit-message.ts +++ b/apps/server/src/routes/worktree/routes/generate-commit-message.ts @@ -11,7 +11,7 @@ import { promisify } from 'util'; import { existsSync } from 'fs'; import { join } from 'path'; import { createLogger } from '@automaker/utils'; -import { DEFAULT_PHASE_MODELS, isCursorModel, stripProviderPrefix } from '@automaker/types'; +import { isCursorModel, stripProviderPrefix } from '@automaker/types'; import { resolvePhaseModel } from '@automaker/model-resolver'; import { mergeCommitMessagePrompts } from '@automaker/prompts'; import { ProviderFactory } from '../../../providers/provider-factory.js'; @@ -162,18 +162,12 @@ export function createGenerateCommitMessageHandler( phaseModel: phaseModelEntry, provider: claudeCompatibleProvider, credentials, - } = settingsService - ? await getPhaseModelWithOverrides( - 'commitMessageModel', - settingsService, - worktreePath, - '[GenerateCommitMessage]' - ) - : { - phaseModel: DEFAULT_PHASE_MODELS.commitMessageModel, - provider: undefined, - credentials: undefined, - }; + } = await getPhaseModelWithOverrides( + 'commitMessageModel', + settingsService, + worktreePath, + '[GenerateCommitMessage]' + ); const { model } = resolvePhaseModel(phaseModelEntry); logger.info( diff --git a/apps/server/src/services/auto-mode-service.ts b/apps/server/src/services/auto-mode-service.ts index a2d7f1af..5d87000b 100644 --- a/apps/server/src/services/auto-mode-service.ts +++ b/apps/server/src/services/auto-mode-service.ts @@ -2318,18 +2318,12 @@ Format your response as a structured markdown document.`; phaseModel: phaseModelEntry, provider: analysisClaudeProvider, credentials, - } = this.settingsService - ? await getPhaseModelWithOverrides( - 'projectAnalysisModel', - this.settingsService, - projectPath, - '[AutoMode]' - ) - : { - phaseModel: DEFAULT_PHASE_MODELS.projectAnalysisModel, - provider: undefined, - credentials: undefined, - }; + } = await getPhaseModelWithOverrides( + 'projectAnalysisModel', + this.settingsService, + projectPath, + '[AutoMode]' + ); const { model: analysisModel, thinkingLevel: analysisThinkingLevel } = resolvePhaseModel(phaseModelEntry); logger.info(