-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Feature/self open ai key #491
base: main
Are you sure you want to change the base?
Conversation
…own APIKEY and Access Token. By setting ENABLE_X_SELF="true" in the .env file, each user can use their own API Key.
…user profile page where users can enter their own OpenAI API key. This allows users to use their own OpenAI key for accessing OpenAI services.
Someone is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces a comprehensive feature for enabling user-specific OpenAI API key management and a new X (Twitter) self-integration provider. The changes span multiple components across the frontend and backend, adding configuration variables, updating database schemas, and implementing new provider functionality. The primary focus is on allowing users to use their personal OpenAI API keys and providing a more flexible social media integration mechanism. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant Backend
participant OpenAI
participant Database
User->>Frontend: Configure OpenAI API Key
Frontend->>Backend: Submit API Key
Backend->>Database: Store User's OpenAI API Key
User->>Frontend: Request AI Copilot
Frontend->>Backend: Request with User Context
Backend->>Database: Retrieve User's API Key
Backend->>OpenAI: Use User's Specific API Key
OpenAI-->>Backend: AI Response
Backend-->>Frontend: Return AI Response
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
libraries/nestjs-libraries/src/upload/cloudflare.storage.ts (1)
Line range hint
86-93
: Implement removeFile method to prevent resource leaks.The commented-out
removeFile
implementation could lead to resource leaks and increased storage costs over time.Consider implementing the method:
async removeFile(filePath: string): Promise<void> { - // const fileName = filePath.split('/').pop(); // Extract the filename from the path - // const command = new DeleteObjectCommand({ - // Bucket: this._bucketName, - // Key: fileName, - // }); - // await this._client.send(command); + const fileName = filePath.split('/').pop(); + if (!fileName) { + throw new Error('Invalid file path'); + } + const command = new DeleteObjectCommand({ + Bucket: this._bucketName, + Key: fileName, + }); + try { + await this._client.send(command); + } catch (e) { + const error = e instanceof Error ? e : new Error('Unknown error during file removal'); + this.logger.error('Failed to remove file from Cloudflare R2', { + error: error.message, + filePath, + fileName, + }); + throw error; + } }
🧹 Nitpick comments (5)
apps/frontend/src/components/launches/providers/xself/xself.provider.tsx (1)
13-14
: UseendsWith
instead ofindexOf
for file extension checksUsing
indexOf('mp4') > -1
can incorrectly match filenames that contain 'mp4' elsewhere in the name (e.g., 'example_mp4_video.txt'). Consider usingendsWith('.mp4')
for accurate file extension checks.Apply the following diff to update the file extension checks:
- (p) => p.some((m) => m.path.indexOf('mp4') > -1) && p.length > 1 + (p) => p.some((m) => m.path.endsWith('.mp4')) && p.length > 1- if (load.indexOf('mp4') > -1) { + if (load.endsWith('.mp4')) {Also applies to: 20-21
apps/backend/src/api/routes/users.controller.ts (1)
55-62
: Consider extracting subscription tier logic.The tier determination logic mixes environment checks with subscription status. Consider extracting this into a separate service method for better maintainability.
- tier: - // @ts-ignore - organization?.subscription?.subscriptionTier || - (!process.env.STRIPE_PUBLISHABLE_KEY ? 'ULTIMATE' : 'FREE'), + tier: this._subscriptionService.determineUserTier(organization?.subscription, process.env.STRIPE_PUBLISHABLE_KEY),libraries/nestjs-libraries/src/database/prisma/integrations/integration.service.ts (1)
413-413
: Consider explicit return type and values.The method implicitly returns undefined in multiple places. Consider adding a return type to the method and being explicit about the return values for better code clarity.
- async processPlugs(data: { + async processPlugs(): Promise<void> { // ... existing code ... - return; + return undefined;Also applies to: 439-439, 443-443
apps/frontend/src/components/layout/settings.component.tsx (1)
80-81
: Remove debug console.log statementThe console.log statement appears to be leftover debugging code and should be removed.
- console.log('zaaa', enabledOpenaiSelf);
libraries/nestjs-libraries/src/database/prisma/migrations/20241213183825_add_open_aiapi_key_to_user/migration.sql (1)
393-400
: Fix typo in table name: "ExisingPlugData" → "ExistingPlugData"There's a typo in the table name. This should be fixed to maintain code quality and prevent confusion.
-CREATE TABLE "ExisingPlugData" ( +CREATE TABLE "ExistingPlugData" (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/frontend/public/icons/platforms/xself.png
is excluded by!**/*.png
📒 Files selected for processing (18)
.env.example
(2 hunks)apps/backend/src/api/routes/copilot.controller.ts
(1 hunks)apps/backend/src/api/routes/users.controller.ts
(2 hunks)apps/frontend/src/app/layout.tsx
(2 hunks)apps/frontend/src/components/launches/providers/show.all.providers.tsx
(2 hunks)apps/frontend/src/components/launches/providers/xself/xself.provider.tsx
(1 hunks)apps/frontend/src/components/layout/layout.settings.tsx
(2 hunks)apps/frontend/src/components/layout/settings.component.tsx
(5 hunks)libraries/nestjs-libraries/src/database/prisma/integrations/integration.service.ts
(3 hunks)libraries/nestjs-libraries/src/database/prisma/migrations/20241213183825_add_open_aiapi_key_to_user/migration.sql
(1 hunks)libraries/nestjs-libraries/src/database/prisma/migrations/migration_lock.toml
(1 hunks)libraries/nestjs-libraries/src/database/prisma/schema.prisma
(2 hunks)libraries/nestjs-libraries/src/database/prisma/users/users.repository.ts
(2 hunks)libraries/nestjs-libraries/src/dtos/users/user.details.dto.ts
(2 hunks)libraries/nestjs-libraries/src/integrations/integration.manager.ts
(1 hunks)libraries/nestjs-libraries/src/integrations/social/xself.provider.ts
(1 hunks)libraries/nestjs-libraries/src/upload/cloudflare.storage.ts
(3 hunks)libraries/react-shared-libraries/src/helpers/variable.context.tsx
(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- libraries/nestjs-libraries/src/database/prisma/migrations/migration_lock.toml
🔇 Additional comments (17)
apps/frontend/src/components/launches/providers/xself/xself.provider.tsx (1)
1-52
: Code implementation is correct and functions as expected
The validation logic and the checkVideoDuration
function are implemented correctly. The code effectively enforces media constraints for posts.
apps/frontend/src/app/layout.tsx (2)
41-41
: LGTM! Environment variable coercion is correct.
The use of !!
operator correctly coerces the ENABLE_OPENAI_SELF
environment variable to a boolean value.
28-28
: LGTM! Favicon link formatting improved.
The favicon link element formatting change improves readability without affecting functionality.
libraries/nestjs-libraries/src/upload/cloudflare.storage.ts (1)
33-35
: LGTM! Robust content-type header access.
The header access is well-implemented, checking both case variants of 'content-type'.
.env.example (1)
77-77
: LGTM! The OpenAI self-key feature toggle is properly configured.
The ENABLE_OPENAI_SELF
environment variable is correctly placed in the Misc Settings section alongside other OpenAI-related configurations, with a safe default value of "false".
apps/frontend/src/components/launches/providers/show.all.providers.tsx (1)
20-20
: LGTM! XSelfProvider integration looks correct.
The XSelfProvider is properly imported and added to the Providers array, maintaining consistency with other provider implementations.
Also applies to: 41-41
libraries/nestjs-libraries/src/integrations/integration.manager.ts (1)
28-28
: Verify the impact of the conditional provider initialization.
The provider selection logic needs careful testing to ensure it works correctly in all environments.
✅ Verification successful
Let me gather more context about the provider implementations and their usage.
Based on the code analysis, I can now provide a final response:
The conditional provider initialization is well-designed and safe to use
The code review reveals that both XProvider
and XSelfProvider
implement the same interface and provide identical functionality with different authentication approaches:
XProvider
: Uses environment variables (X_API_KEY
,X_API_SECRET
) for app credentialsXSelfProvider
: Uses user-provided API credentials through custom fields
The conditional initialization is safe because:
- Both providers implement the same
SocialProvider
interface - They share identical posting, auto-repost, and auto-plug functionality
- The only difference is in how they handle authentication and API credentials
- The toggle is used only once at initialization, reducing complexity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with X provider initialization
# Check if XSelfProvider is used in other places
rg -l "XSelfProvider"
# Check for environment variable usage patterns
rg "ENABLE_X_SELF"
# Look for provider initialization patterns
ast-grep --pattern 'new XProvider()'
ast-grep --pattern 'new XSelfProvider()'
Length of output: 776
Script:
#!/bin/bash
# Check the implementation of both providers
cat libraries/nestjs-libraries/src/integrations/social/xself.provider.ts
cat libraries/nestjs-libraries/src/integrations/social/x.provider.ts
# Check if there are any tests related to these providers
rg -l "XProvider.test|XProvider.spec"
rg -l "XSelfProvider.test|XSelfProvider.spec"
# Check for any configuration files that might set ENABLE_X_SELF
fd -e env -e config -e yaml -e yml | xargs rg "ENABLE_X_SELF"
Length of output: 16551
libraries/nestjs-libraries/src/database/prisma/users/users.repository.ts (1)
131-131
: LGTM! Changes align with the feature requirements.
The addition of openAIAPIKey
to both getPersonal
and changePersonal
methods is implemented correctly and consistently.
Also applies to: 152-152
apps/backend/src/api/routes/users.controller.ts (1)
45-45
: Verify the necessity of the Request parameter.
The @Req()
parameter has been added but its usage appears to be only for accessing cookies. Consider using the more specific @Cookies()
decorator if available in your framework, or document why the full request object is needed.
✅ Verification successful
Let me verify if there are any other usages of the Request parameter or if there's a specific reason for using it over cookies.
Let me try a different pattern to find the method implementation.
The Request parameter is necessary for accessing impersonation cookies
The @Req()
parameter in the getSelf
method is specifically used to check the impersonation state through req.cookies.impersonate
, which is a legitimate use case that cannot be replaced with a simpler @Cookies()
decorator. The request object's cookies are used to determine if the user is in impersonation mode, which is then included in the response as impersonate: !!req.cookies.impersonate
.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the Request object is used for anything other than cookies
rg -A 5 "req\." apps/backend/src/api/routes/users.controller.ts
Length of output: 199
Script:
#!/bin/bash
# Check the full implementation of getSelf method
ast-grep --pattern 'getSelf($$$) {
$$$
}'
Length of output: 45
Script:
#!/bin/bash
# Check the full implementation of getSelf method with class context
ast-grep --pattern 'class $_ {
$$$
getSelf($$$) {
$$$
}
$$$
}'
# Also get the full file content to see the method implementation
cat apps/backend/src/api/routes/users.controller.ts
Length of output: 20366
libraries/nestjs-libraries/src/database/prisma/schema.prisma (1)
45-45
: LGTM! The schema change looks good.
The addition of the optional openAIAPIKey
field to the User model is appropriate for storing user-specific OpenAI API keys.
Let's verify if there are any migration files for this schema change:
✅ Verification successful
Migration file exists and matches the schema change
The migration file 20241213183825_add_open_aiapi_key_to_user
has been properly created for adding the openAIAPIKey
field to the User model. The migration is recent (December 13, 2024) and corresponds to the schema change being reviewed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for migration files related to the openAIAPIKey addition
fd --type f ".*openai.*\.sql$" --exec cat {} \;
Length of output: 52
Script:
#!/bin/bash
# Let's check for any migration files in common migration directories
fd --type f "\.sql$" --exec cat {} \;
# Also check for Prisma migration files
fd --type f "migration\.sql$" --exec cat {} \;
Length of output: 48500
Script:
#!/bin/bash
# Let's check for any Prisma migration files in the migrations directory
fd --type d "migrations" --exec ls -la {} \;
# Also check for any recent migration files
fd --type f --changed-within 2weeks ".*\.sql$" --exec ls -la {} \;
Length of output: 556
apps/frontend/src/components/layout/settings.component.tsx (2)
197-201
: LGTM! Proper feature flag implementation.
The OpenAI API Key input field is correctly conditionally rendered based on the enabledOpenaiSelf
flag.
161-164
: LGTM! Improved event handler placement.
The onClick handler is now properly placed on the text element rather than the parent button.
apps/frontend/src/components/layout/layout.settings.tsx (2)
135-138
: LGTM! Improved formatting.
The systray-buttons div formatting has been improved for better readability.
90-90
: Verify the logo display logic change
The condition for displaying the logo has been inverted from isGeneral ? '/postiz.svg' : '/logo.svg'
to !isGeneral ? '/postiz.svg' : '/logo.svg'
. Please confirm this change is intentional and matches the expected behavior.
✅ Verification successful
Logo display logic change is consistent with server-side implementation
The change in logo display logic (!isGeneral ? '/postiz.svg' : '/logo.svg'
) is correct as it maintains consistency with the server-side implementation in auth/layout.tsx
. Both files now follow the same pattern where:
- When
isGeneral
is false, it showspostiz.svg
- When
isGeneral
is true, it showslogo.svg
This is also consistent with other usages of isGeneral
in the same component for styling and conditional rendering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other instances of logo display logic
rg -A 2 "postiz\.svg|logo\.svg"
Length of output: 583
Script:
#!/bin/bash
# Let's check the git history for this specific line to understand the change
git log -L ':apps/frontend/src/components/layout/layout.settings.tsx' | grep -A 5 "src="
# Also, let's check the isGeneral usage in both files
rg "isGeneral" apps/frontend/src/components/layout/layout.settings.tsx -A 2 -B 2
rg "isGeneralServerSide" apps/frontend/src/app/auth/layout.tsx -A 2 -B 2
Length of output: 2196
libraries/nestjs-libraries/src/database/prisma/migrations/20241213183825_add_open_aiapi_key_to_user/migration.sql (3)
38-63
: LGTM: User table schema changes look good
The addition of the nullable openAIAPIKey
field to the User table is well-designed:
- TEXT type is appropriate for storing API keys
- Nullable constraint aligns with the optional nature of the feature
- No index on the API key field, which is correct as it won't be used for searching
609-727
: LGTM: Database schema design follows best practices
The foreign key constraints and relationships are well-designed:
- Appropriate use of ON DELETE RESTRICT for required relationships
- Correct use of ON DELETE SET NULL for optional relationships
- Proper referential integrity maintained throughout the schema
402-419
: LGTM: Index design is well-optimized
The indexing strategy for the User table and related entities is well-thought-out:
- Appropriate indexes on frequently queried fields
- Composite unique index on email and providerName prevents duplicate accounts
- No unnecessary indexes that could slow down write operations
let openAIAPIKey = ''; | ||
|
||
//Check if OPEN AI is enabled for Self tokens. | ||
if (process.env.ENABLE_OPENAI_SELF === 'true') { | ||
const userPersonal = await this._userService.getPersonal(user.id); | ||
openAIAPIKey = userPersonal.openAIAPIKey; | ||
} else { | ||
openAIAPIKey = process.env.OPENAI_API_KEY; | ||
} | ||
|
||
if (openAIAPIKey === undefined || openAIAPIKey === '') { | ||
Logger.warn('OpenAI API key not set, chat functionality will not work'); | ||
return | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle missing OpenAI API key by returning an appropriate HTTP response
When the OpenAI API key is missing or invalid, the method logs a warning and returns without sending a response to the client. This may cause the client request to hang or receive an unexpected result. It's important to inform the client of the issue.
Apply this diff to return a 400 Bad Request response with an error message:
if (openAIAPIKey === undefined || openAIAPIKey === '') {
Logger.warn('OpenAI API key not set, chat functionality will not work');
+ res.status(400).json({ error: 'OpenAI API key is not set or invalid.' });
+ return;
}
Committable suggestion skipped: line range outside the PR's diff.
async authenticate(params: { code: string; codeVerifier: string }) { | ||
const body = JSON.parse(Buffer.from(params.code, 'base64').toString()); | ||
|
||
const { code, codeVerifier } = params; | ||
const [oauth_token, oauth_token_secret] = codeVerifier.split(':'); | ||
|
||
const startingClient = new TwitterApi({ | ||
appKey: body.apiKey, | ||
appSecret: body.apiSecretKey, | ||
accessToken: body.accessToken, | ||
accessSecret: body.accessTokenSecret, | ||
}); | ||
|
||
const { id, name, profile_image_url_https, screen_name } = | ||
await startingClient.currentUser(true); | ||
|
||
return { | ||
id: String(id), | ||
accessToken: | ||
body.apiKey + | ||
':' + | ||
body.apiSecretKey + | ||
':' + | ||
body.accessToken + | ||
':' + | ||
body.accessTokenSecret, | ||
|
||
name, | ||
refreshToken: '', | ||
expiresIn: 999999999, | ||
picture: profile_image_url_https, | ||
username: screen_name, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate inputs and securely handle user API credentials
The authenticate
method processes user-provided API keys and tokens without input validation or error handling. This poses security risks, including potential injection attacks and credential leakage.
Apply the following changes to enhance security:
-
Input Validation and Error Handling:
async authenticate(params: { code: string; codeVerifier: string }) {
-
if (!params.code || !params.codeVerifier) {
-
throw new Error('Missing required authentication parameters.');
-
}
-
let body;
-
try {
-
body = JSON.parse(Buffer.from(params.code, 'base64').toString());
-
} catch (error) {
-
throw new Error('Invalid code parameter format.');
-
}
-
if (
-
!body.apiKey ||
-
!body.apiSecretKey ||
-
!body.accessToken ||
-
!body.accessTokenSecret
-
) {
-
throw new Error('Incomplete API credentials provided.');
-
}
const { codeVerifier } = params;
const [oauth_token, oauth_token_secret] = codeVerifier.split(':');
- **Secure Storage of Credentials:**
Ensure that API keys and tokens are encrypted before storage and decrypted only when in use.
- **Avoid Logging Sensitive Data:**
Review the code for any logging statements that might output sensitive information and remove or sanitize them.
- **Implement Access Controls:**
Restrict access to methods handling sensitive data to authorized users only.
> Committable suggestion skipped: line range outside the PR's diff.
<!-- This is an auto-generated comment by CodeRabbit -->
@IsString() | ||
@IsOptional() | ||
openAIAPIKey: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid exposing sensitive openAIAPIKey
in UserDetailDto
Including the openAIAPIKey
in UserDetailDto
may inadvertently expose the user's API key when user details are sent to the frontend, posing a security risk.
Consider the following steps:
- Remove
openAIAPIKey
fromUserDetailDto
if it's not necessary to send this data to the client. - If users need to input their API key, handle it separately and ensure it's not included in responses.
- Ensure that any transmission of the API key is done securely (e.g., over HTTPS) and that the key is stored encrypted on the server.
@@ -5,9 +5,10 @@ import { createContext, FC, ReactNode, useContext, useEffect } from 'react'; | |||
interface VariableContextInterface { | |||
billingEnabled: boolean; | |||
isGeneral: boolean; | |||
enabledOpenaiSelf: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
The new context property is properly used but has a debug console.log statement
The enabledOpenaiSelf
property is correctly integrated and used in appropriate locations:
- Initialized in layout with an environment variable
- Used in settings component for conditional rendering
- Properly defined in the context interface
However, there's a debug console.log
statement that should be removed:
apps/frontend/src/components/layout/settings.component.tsx
: Removeconsole.log('zaaa', enabledOpenaiSelf);
🔗 Analysis chain
LGTM! Verify usage of the new context property.
The addition of enabledOpenaiSelf
to the context interface and its initialization is well-implemented with appropriate typing and a secure default value of false
.
Let's verify all components that consume this new context property:
Also applies to: 20-20
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using the enabledOpenaiSelf property
rg "enabledOpenaiSelf" --type tsx --type ts
Length of output: 71
Script:
#!/bin/bash
# Search for components using the enabledOpenaiSelf property with proper file extensions
rg "enabledOpenaiSelf" -t js -t jsx -t ts -t tsx
Length of output: 76
Script:
#!/bin/bash
# Search for components using the enabledOpenaiSelf property without file type restrictions
rg "enabledOpenaiSelf"
Length of output: 622
try { | ||
await this._client.send(command); | ||
} catch (e) { | ||
console.log(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling in uploadSimple method.
The current error handling simply logs to console and swallows the error. This could mask issues and make debugging difficult.
Consider implementing proper error handling:
try {
await this._client.send(command);
} catch (e) {
- console.log(e);
+ const error = e instanceof Error ? e : new Error('Unknown error during upload');
+ this.logger.error('Failed to upload file to Cloudflare R2', {
+ error: error.message,
+ path,
+ id,
+ });
+ throw error;
}
Also consider:
- Adding retry logic for transient failures
- Adding metrics/monitoring for upload failures
- Implementing proper logging using a logger service instead of console.log
Committable suggestion skipped: line range outside the PR's diff.
#Enable X Integration with Self Generated Tokens. | ||
ENABLE_X_SELF="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the default value and placement of X self-integration toggle.
A few concerns about the ENABLE_X_SELF
configuration:
- Default value is set to "true" which might not be safe for production environments
- It's placed at the end of the file instead of being grouped with other X-related settings (lines 27-31)
Consider these changes:
-#Enable X Integration with Self Generated Tokens.
-ENABLE_X_SELF="true"
+# X Settings
X_API_KEY=""
X_API_SECRET=""
X_CLIENT=""
X_SECRET=""
+ENABLE_X_SELF="false" # Enable X Integration with Self Generated Tokens
Committable suggestion skipped: line range outside the PR's diff.
@@ -21,10 +21,11 @@ import { DiscordProvider } from '@gitroom/nestjs-libraries/integrations/social/d | |||
import { SlackProvider } from '@gitroom/nestjs-libraries/integrations/social/slack.provider'; | |||
import { MastodonProvider } from '@gitroom/nestjs-libraries/integrations/social/mastodon.provider'; | |||
import { BlueskyProvider } from '@gitroom/nestjs-libraries/integrations/social/bluesky.provider'; | |||
import { XSelfProvider } from '@gitroom/nestjs-libraries/integrations/social/xself.provider'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider improving the provider initialization logic.
The current implementation has several potential issues:
- The provider selection is fixed at startup and cannot be changed dynamically
- The environment variable check is not type-safe
- No fallback handling if the environment variable is undefined
Consider this more robust implementation:
- process.env.ENABLE_X_SELF === 'true' ? new XSelfProvider() : new XProvider(),
+ process.env.ENABLE_X_SELF?.toLowerCase() === 'true' ? new XSelfProvider() : new XProvider(),
Also, consider implementing a factory pattern to allow dynamic provider switching:
class XProviderFactory {
static createProvider(enableSelf: boolean): SocialProvider {
return enableSelf ? new XSelfProvider() : new XProvider();
}
}
Also applies to: 28-28
console.log('XXXX UPLOADING'); | ||
const uploadedPicture = picture | ||
? await this.storage.uploadSimple(picture) | ||
: undefined; | ||
console.log('XXXX UPLOAD DONE'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove debug console.log statements.
Debug console.log statements should not be committed to production code. Consider replacing them with proper logging using a logging service if upload tracking is needed.
- console.log('XXXX UPLOADING');
const uploadedPicture = picture
? await this.storage.uploadSimple(picture)
: undefined;
- console.log('XXXX UPLOAD DONE');
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
console.log('XXXX UPLOADING'); | |
const uploadedPicture = picture | |
? await this.storage.uploadSimple(picture) | |
: undefined; | |
console.log('XXXX UPLOAD DONE'); | |
const uploadedPicture = picture | |
? await this.storage.uploadSimple(picture) | |
: undefined; |
"lastName" TEXT, | ||
"isSuperAdmin" BOOLEAN NOT NULL DEFAULT false, | ||
"bio" TEXT, | ||
"openAIAPIKey" TEXT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security: Encrypt OpenAI API keys at rest
The openAIAPIKey
field currently stores API keys as plain text. API keys should be encrypted at rest to prevent unauthorized access in case of a data breach.
Consider implementing one of these approaches:
- Use a dedicated encryption service
- Leverage database-level encryption (e.g., PostgreSQL's pgcrypto)
- Implement application-level encryption before storing the keys
Would you like me to provide a detailed implementation guide for any of these approaches?
What kind of change does this PR introduce?
eg: feature
Why was this change needed?
New feature: Use ENABLE_OPENAI_SELF="true" to display a field on the user profile page where users can enter their own OpenAI API key. This allows users to use their own OpenAI key for accessing OpenAI services.
Other information:
eg: Did you discuss this change with anybody before working on it (not required, but can be a good idea for bigger changes). Any plans for the future, etc?
No.
Checklist:
Put a "X" in the boxes below to indicate you have followed the checklist;
Summary by CodeRabbit
New Features
XSelfProvider
, for enhanced social media integration.Bug Fixes
Documentation
.env.example
.Refactor
Chores