Skip to content

Commit 5804b68

Browse files
committed
refactor(embedding): fix GitHub Copilot review issues in OpenAI embedding
- Fix undefined property references (this.dimensionDetected → this.isDimensionDetected) - Unify duplicate state management (single isDimensionDetected flag and dimensionDetectionPromise) - Replace console.log with production-safe logging (environment-controlled log() method) - Maintain 100% test coverage (20/20 tests passing) - Preserve backward compatibility for OpenAI and OAPI Ollama forwarding
1 parent 4c05d6c commit 5804b68

File tree

1 file changed

+36
-29
lines changed

1 file changed

+36
-29
lines changed

packages/core/src/embedding/openai-embedding.ts

Lines changed: 36 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,10 @@ export class OpenAIEmbedding extends Embedding {
1616
private client: OpenAI;
1717
private config: OpenAIEmbeddingConfig;
1818
private dimension: number = 1536; // Default dimension for text-embedding-3-small
19-
private dimensionDetected: boolean = false; // Track if dimension has been detected
20-
private dimensionDetectionPromise: Promise<number> | null = null; // Track detection process
19+
private isDimensionDetected: boolean = false; // Track if dimension has been detected for any provider type
20+
private dimensionDetectionPromise: Promise<number> | null = null; // Track detection process (unified for all providers)
2121
protected maxTokens: number = 8192; // Maximum tokens for OpenAI embedding models
2222
private isOllamaViaOAPI: boolean = false; // Whether using Ollama model via OAPI
23-
private isOllamaDimensionDetected: boolean = false; // Track if OAPI dimension has been detected
24-
private ollamaDimensionDetectionPromise: Promise<number> | null = null; // Track OAPI detection process
2523

2624
constructor(config: OpenAIEmbeddingConfig) {
2725
super();
@@ -40,19 +38,30 @@ export class OpenAIEmbedding extends Embedding {
4038
});
4139

4240
if (this.isOllamaViaOAPI) {
43-
console.log(`[OpenAI] Configured for Ollama model ${config.model} via OAPI forwarding`);
41+
this.log(`Configured for Ollama model ${config.model} via OAPI forwarding`);
4442
// Reset dimension since Ollama models have different dimensions
4543
this.dimension = DEFAULT_OLLAMA_DIMENSION; // Common Ollama embedding dimension
4644
} else {
4745
// Set dimension detection flag for known models
4846
const knownModels = OpenAIEmbedding.getSupportedModels();
4947
if (knownModels[config.model]) {
5048
this.dimension = knownModels[config.model].dimension;
51-
this.dimensionDetected = true;
49+
this.isDimensionDetected = true;
5250
}
5351
}
5452
}
5553

54+
/**
55+
* Internal logging method that can be easily controlled
56+
*/
57+
private log(message: string): void {
58+
// In production, this could be replaced with proper logging library
59+
// For now, only log if debugging is enabled
60+
if (process.env.NODE_ENV !== 'production' || process.env.DEBUG_EMBEDDINGS) {
61+
console.log(`[OpenAI] ${message}`);
62+
}
63+
}
64+
5665
/**
5766
* Correct baseURL by adding /v1 if needed for OpenAI compatibility
5867
*/
@@ -67,7 +76,7 @@ export class OpenAIEmbedding extends Embedding {
6776
// For custom endpoints, ensure /v1 path is present
6877
if (!baseURL.endsWith('/v1') && !baseURL.includes('/v1/')) {
6978
const normalizedURL = baseURL.endsWith('/') ? baseURL.slice(0, -1) : baseURL;
70-
console.log(`[OpenAI] Auto-correcting baseURL: ${baseURL}${normalizedURL}/v1`);
79+
this.log(`Auto-correcting baseURL: ${baseURL}${normalizedURL}/v1`);
7180
return `${normalizedURL}/v1`;
7281
}
7382

@@ -121,7 +130,7 @@ export class OpenAIEmbedding extends Embedding {
121130
* Detect dimension for Ollama models accessed via OAPI
122131
*/
123132
private async detectOllamaDimensionViaOAPI(testText: string, model: string): Promise<number> {
124-
console.log(`[OpenAI] Detecting Ollama model dimension via OAPI for ${model}...`);
133+
this.log(`Detecting Ollama model dimension via OAPI for ${model}...`);
125134

126135
try {
127136
const processedText = this.preprocessText(testText);
@@ -136,7 +145,7 @@ export class OpenAIEmbedding extends Embedding {
136145
}
137146

138147
const dimension = response.data[0].embedding.length;
139-
console.log(`[OpenAI] Detected Ollama dimension via OAPI: ${dimension}`);
148+
this.log(`Detected Ollama dimension via OAPI: ${dimension}`);
140149
return dimension;
141150
} catch (error) {
142151
const errorMessage = error instanceof Error ? error.message : 'Unknown error';
@@ -157,8 +166,8 @@ export class OpenAIEmbedding extends Embedding {
157166
const knownModels = OpenAIEmbedding.getSupportedModels();
158167
if (knownModels[model] && this.dimension !== knownModels[model].dimension) {
159168
this.dimension = knownModels[model].dimension;
160-
this.dimensionDetected = true;
161-
} else if (!knownModels[model] && !this.dimensionDetected) {
169+
this.isDimensionDetected = true;
170+
} else if (!knownModels[model] && !this.isDimensionDetected) {
162171
await this.ensureDimensionDetected(model);
163172
}
164173

@@ -192,7 +201,7 @@ export class OpenAIEmbedding extends Embedding {
192201
const model = this.config.model;
193202

194203
// Detect dimension if not already detected for Ollama
195-
if (!this.isOllamaDimensionDetected) {
204+
if (!this.isDimensionDetected) {
196205
await this.ensureOllamaDimensionDetected(model);
197206
}
198207

@@ -230,8 +239,8 @@ export class OpenAIEmbedding extends Embedding {
230239
const knownModels = OpenAIEmbedding.getSupportedModels();
231240
if (knownModels[model] && this.dimension !== knownModels[model].dimension) {
232241
this.dimension = knownModels[model].dimension;
233-
this.dimensionDetected = true;
234-
} else if (!knownModels[model] && !this.dimensionDetected) {
242+
this.isDimensionDetected = true;
243+
} else if (!knownModels[model] && !this.isDimensionDetected) {
235244
await this.ensureDimensionDetected(model);
236245
}
237246

@@ -261,13 +270,13 @@ export class OpenAIEmbedding extends Embedding {
261270
* Batch embed using Ollama model via OAPI forwarding
262271
*/
263272
private async embedBatchOllamaViaOAPI(texts: string[]): Promise<EmbeddingVector[]> {
264-
console.log(`[OpenAI] Batch embedding ${texts.length} texts with Ollama model ${this.config.model} via OAPI...`);
273+
this.log(`Batch embedding ${texts.length} texts with Ollama model ${this.config.model} via OAPI...`);
265274

266275
const processedTexts = this.preprocessTexts(texts);
267276
const model = this.config.model;
268277

269278
// Detect dimension if not already detected for Ollama
270-
if (!this.isOllamaDimensionDetected) {
279+
if (!this.isDimensionDetected) {
271280
await this.ensureOllamaDimensionDetected(model);
272281
}
273282

@@ -321,17 +330,15 @@ export class OpenAIEmbedding extends Embedding {
321330
this.config.model = model;
322331

323332
// Reset all detection states
324-
this.dimensionDetected = false;
325-
this.isOllamaDimensionDetected = false;
333+
this.isDimensionDetected = false;
326334
this.dimensionDetectionPromise = null;
327-
this.ollamaDimensionDetectionPromise = null;
328335

329336
const knownModels = OpenAIEmbedding.getSupportedModels();
330337

331338
if (knownModels[model] && !this.isOllamaViaOAPI) {
332339
// Known OpenAI model
333340
this.dimension = knownModels[model].dimension;
334-
this.dimensionDetected = true;
341+
this.isDimensionDetected = true;
335342
} else {
336343
// Unknown model or OAPI model - detect dimension
337344
if (this.isOllamaViaOAPI) {
@@ -392,7 +399,7 @@ export class OpenAIEmbedding extends Embedding {
392399
* Ensure dimension is detected for standard OpenAI models (race condition safe)
393400
*/
394401
private async ensureDimensionDetected(model: string): Promise<void> {
395-
if (this.dimensionDetected) {
402+
if (this.isDimensionDetected) {
396403
return;
397404
}
398405

@@ -404,7 +411,7 @@ export class OpenAIEmbedding extends Embedding {
404411
this.dimensionDetectionPromise = this.detectDimension();
405412
try {
406413
this.dimension = await this.dimensionDetectionPromise;
407-
this.dimensionDetected = true;
414+
this.isDimensionDetected = true;
408415
} finally {
409416
this.dimensionDetectionPromise = null;
410417
}
@@ -414,21 +421,21 @@ export class OpenAIEmbedding extends Embedding {
414421
* Ensure OAPI dimension is detected for Ollama models (race condition safe)
415422
*/
416423
private async ensureOllamaDimensionDetected(model: string): Promise<void> {
417-
if (this.isOllamaDimensionDetected) {
424+
if (this.isDimensionDetected) {
418425
return;
419426
}
420427

421-
if (this.ollamaDimensionDetectionPromise) {
422-
await this.ollamaDimensionDetectionPromise;
428+
if (this.dimensionDetectionPromise) {
429+
await this.dimensionDetectionPromise;
423430
return;
424431
}
425432

426-
this.ollamaDimensionDetectionPromise = this.detectOllamaDimensionViaOAPI('test', model);
433+
this.dimensionDetectionPromise = this.detectOllamaDimensionViaOAPI('test', model);
427434
try {
428-
this.dimension = await this.ollamaDimensionDetectionPromise;
429-
this.isOllamaDimensionDetected = true;
435+
this.dimension = await this.dimensionDetectionPromise;
436+
this.isDimensionDetected = true;
430437
} finally {
431-
this.ollamaDimensionDetectionPromise = null;
438+
this.dimensionDetectionPromise = null;
432439
}
433440
}
434441
}

0 commit comments

Comments
 (0)