Skip to content

Add support for OpenAI-compatible API provider and optional base URL #68

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nathanredblur
Copy link

Add support to use openAI compatible providers.
fix error on parse JSON responses.

Copy link
Collaborator

@bhaumikmaan bhaumikmaan left a comment

Choose a reason for hiding this comment

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

I would want an overall review of the need of the PR and the changes made.
Blindly reviewing doesn't help much tbh but needs some changes

@@ -20,6 +21,7 @@ export class ConfigHelper extends EventEmitter {
private defaultConfig: Config = {
apiKey: "",
apiProvider: "gemini", // Default to Gemini
baseUrl: "", // Optional custom API URL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about this change
seems like it beats the point of the default model

updates.extractionModel = "Claude#claude-3-7-sonnet-20250219";
updates.solutionModel = "Claude#claude-3-7-sonnet-20250219";
updates.debuggingModel = "Claude#claude-3-7-sonnet-20250219";
} else if (updates.apiProvider === "openai-compatible") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If updates.apiProvider === "openai-compatible"
why are the models for it claude-3-7-sonnet-20250219

Comment on lines +510 to +514
const jsonRegex = /```json\n([\s\S]*?)\n```/;
const match = responseText.match(jsonRegex);
if (match && match[1]) {
problemInfo = JSON.parse(match[1]);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What was the need of the change here?
Also I think

if (match && match[1]) {
             problemInfo = JSON.parse(match[1]);
           }

there's a better way to do this matching

Comment on lines +257 to +261
} else if (provider === "openai-compatible") {
// For OpenAI-compatible APIs, we'll keep the current models or set defaults
if (!extractionModel) setExtractionModel("gpt-3.5-turbo");
if (!solutionModel) setSolutionModel("gpt-3.5-turbo");
if (!debuggingModel) setDebuggingModel("gpt-3.5-turbo");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we are creating a new model but in ProcessingHelper.ts here
you are using the same client? So why not bifurcate that too

@@ -325,9 +334,9 @@ export function SettingsDialog({ open: externalOpen, onOpenChange }: SettingsDia
{/* API Provider Selection */}
<div className="space-y-2">
<label className="text-sm font-medium text-white">API Provider</label>
<div className="flex gap-2">
<div className="grid grid-cols-2 gap-2 mb-2">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Comment on lines +398 to +417
<div
className={`p-2 rounded-lg cursor-pointer transition-colors ${
apiProvider === "openai-compatible"
? "bg-white/10 border border-white/20"
: "bg-black/30 border border-white/5 hover:bg-white/5"
}`}
onClick={() => handleProviderChange("openai-compatible")}
>
<div className="flex items-center gap-2">
<div
className={`w-3 h-3 rounded-full ${
apiProvider === "openai-compatible" ? "bg-white" : "bg-white/20"
}`}
/>
<div className="flex flex-col">
<p className="font-medium text-white text-sm">OpenAI Compatible</p>
<p className="text-xs text-white/60">Open Router, etc.</p>
</div>
</div>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again here, I would rather have you support a new model with everything made specifically for that model rather than creating new at some places and making changes in others

<div className="mt-2 p-2 rounded-md bg-white/5 border border-white/10">
<p className="text-xs text-white/80 mb-1">Don't have an API key?</p>
{apiProvider === "openai" ? (
{apiProvider === "openai" && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

???

Comment on lines +607 to +618
{apiProvider === "openai-compatible" && (
<Input
id="extractionModelCustom"
type="text"
value={currentValue}
onChange={(e) => setValue(e.target.value)}
placeholder="gpt-3.5-turbo"
className="bg-black/50 border-white/10 text-white"
/>
)}

{apiProvider !== "openai-compatible" && models.map((m) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why separate handling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants