Conversation
|
Needs better null handling |
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenAI integration to use the Chat Completions endpoint, addressing Issue #16 where chat models were being sent to the legacy /v1/completions endpoint and returning a 404.
Changes:
- Switch OpenAI requests from
completions.createtochat.completions.createand update response parsing accordingly. - Bump the extension version and upgrade the
openainpm dependency. - Update
package-lock.jsonto reflect dependency graph changes from theopenaiupgrade.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/openai-service.ts | Migrates revision/translation calls to chat.completions and reads output from choices[0].message.content. |
| package.json | Bumps extension version to 0.0.7 and upgrades openai to ^4.70.0. |
| package-lock.json | Updates locked openai version and transitive dependency set after upgrade. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let result = response.choices[0].message.content; | ||
| return result ?? "null response lol"; |
There was a problem hiding this comment.
response.choices[0].message.content can be null/undefined (and choices[0] can be missing). Returning the literal fallback string "null response lol" will insert user-visible garbage and is unprofessional. Please add a proper guard (e.g., check choices.length and message.content) and either return undefined per the interface or throw a descriptive error/message when the API returns no content.
| let result = response.choices[0].message.content; | ||
| // FIX ME: Not the best null handling... | ||
| return result ?? "null response lol"; |
There was a problem hiding this comment.
Leaving a TODO-style comment (// FIX ME: Not the best null handling...) alongside the placeholder fallback string makes this behavior knowingly incorrect. Please implement proper empty-response handling here as well (guard missing choices/content and return undefined or surface a clear error to the user), and remove the TODO/placeholder text.
Resolve error described in #16