feat: Implement RedditAccountManager for managing Reddit accounts #17
Conversation
…h decryption and usage tracking
WalkthroughAdds a new RedditAccountManager service that loads encrypted Reddit account records from Redis, decrypts credentials, derives account IDs, retrieves per-account usage stats, and exposes methods to list accounts with usage and to track usage updates. Exports the RedditAccount interface, the RedditAccountManager class, and a singleton instance. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Manager as RedditAccountManager
participant Redis as Redis
participant Crypto as Decryption
App->>Manager: getAllAccountsUsage()
activate Manager
Manager->>Redis: connect (if needed)
Manager->>Redis: HGETALL reddit-accounts
Redis-->>Manager: Encrypted account map
loop for each account
Manager->>Crypto: decrypt(credentials)
Crypto-->>Manager: plaintext credentials
Manager->>Redis: HGET reddit_accounts:<accountId> {total_requests,last_request}
Redis-->>Manager: usage fields
Note right of Manager: Derive accountId=<code>reddit_<username></code><br/>Assemble RedditAccount object
end
Manager-->>App: RedditAccount[] (with usage)
deactivate Manager
sequenceDiagram
autonumber
actor App
participant Manager as RedditAccountManager
participant Redis as Redis
App->>Manager: trackApiKeyUsageLocal(accountId)
activate Manager
Manager->>Redis: HINCRBY reddit_accounts:<accountId> total_requests 1
Manager->>Redis: HSET reddit_accounts:<accountId> last_request=<timestamp>
Manager-->>App: ack
deactivate Manager
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/services/redditAccountManager.ts (2)
61-61: Consider sanitizing username for accountId generation.The username is used directly in the
accountIdwithout sanitization. If a Reddit username contains special characters (e.g., underscores, hyphens), this could create confusing Redis keys or potential conflicts.Consider applying this diff to sanitize the username:
- const accountId = `reddit_${credentials.REDDIT_USERNAME}`; + const accountId = `reddit_${credentials.REDDIT_USERNAME.toLowerCase().replace(/[^a-z0-9_-]/g, '_')}`;
107-107: Consider removing redundantaccount_idfield.The
account_idis stored in the hash on line 107, but it's already part of the Redis key name. This redundancy wastes storage space. Unless there's a specific need to query this field independently, consider removing it.Apply this diff if the field is not needed:
- .hSet(key, { last_request: now, account_id: accountId }) + .hSet(key, { last_request: now })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/redditAccountManager.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/redditAccountManager.ts (2)
src/services/BaseAccountManager.ts (1)
BaseAccount(3-8)src/lib/encryption.ts (1)
decrypt(27-39)
🔇 Additional comments (3)
src/services/redditAccountManager.ts (3)
16-26: LGTM!The class declaration and constructor are correctly implemented. The protected properties follow the expected pattern for extending
BaseAccountManager.
111-116: LGTM!The method correctly delegates to
fetchAllAccounts(), which already includes usage statistics. The separate method name improves API clarity for callers who specifically want usage data.
119-120: LGTM!Exporting a singleton instance is a common and appropriate pattern for service managers like this.
| export interface RedditAccount extends BaseAccount { | ||
| accountId: string; | ||
| credentials: { | ||
| REDDIT_CLIENT_ID: string; | ||
| REDDIT_CLIENT_SECRET: string; | ||
| REDDIT_REFRESH_TOKEN: string; | ||
| REDDIT_USERNAME: string; | ||
| }; | ||
| lastUsed?: string; | ||
| totalRequests?: number; | ||
| } |
There was a problem hiding this comment.
Remove redundant field declarations.
The accountId, lastUsed, and totalRequests fields are already defined in the BaseAccount interface (see src/services/BaseAccountManager.ts). Redeclaring them here is unnecessary and may confuse maintainers about which definition applies.
Apply this diff to remove the redundant declarations:
export interface RedditAccount extends BaseAccount {
- accountId: string;
credentials: {
REDDIT_CLIENT_ID: string;
REDDIT_CLIENT_SECRET: string;
REDDIT_REFRESH_TOKEN: string;
REDDIT_USERNAME: string;
};
- lastUsed?: string;
- totalRequests?: number;
}🤖 Prompt for AI Agents
In src/services/redditAccountManager.ts around lines 4 to 14, the RedditAccount
interface is re-declaring accountId, lastUsed, and totalRequests which are
already provided by BaseAccount; remove those three redundant field declarations
from the RedditAccount interface so it only adds the credentials block (and any
fields not present on BaseAccount), keeping the interface as an extension of
BaseAccount with just the Reddit-specific properties.
| for (let i = 0; i < encryptedAccounts.length; i++) { | ||
| const encryptedAccount = encryptedAccounts[i]; | ||
|
|
||
| try { | ||
| // Decrypt credentials | ||
| const credentials = { | ||
| REDDIT_CLIENT_ID: decrypt(encryptedAccount.REDDIT_CLIENT_ID), | ||
| REDDIT_CLIENT_SECRET: decrypt(encryptedAccount.REDDIT_CLIENT_SECRET), | ||
| REDDIT_REFRESH_TOKEN: decrypt(encryptedAccount.REDDIT_REFRESH_TOKEN), | ||
| REDDIT_USERNAME: decrypt(encryptedAccount.REDDIT_USERNAME) | ||
| }; | ||
|
|
||
| // Generate account ID from username (for uniqueness) | ||
| const accountId = `reddit_${credentials.REDDIT_USERNAME}`; | ||
|
|
||
| // Get usage statistics from Redis | ||
| const usage = await this.getApiKeyUsageLocal(accountId); | ||
|
|
||
| accounts.push({ | ||
| accountId, | ||
| credentials, | ||
| lastUsed: usage.last_request || undefined, | ||
| totalRequests: usage.total_requests | ||
| }); | ||
| } catch (e) { | ||
| console.warn(`Failed to decrypt Reddit account ${i + 1}:`, e); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Add validation before accessing encrypted account fields.
The code accesses properties like encryptedAccount.REDDIT_CLIENT_ID without first verifying they exist. If the Redis data is malformed or missing required fields, this will throw unhelpful errors.
Apply this diff to add field validation:
for (let i = 0; i < encryptedAccounts.length; i++) {
const encryptedAccount = encryptedAccounts[i];
try {
+ // Validate required fields exist
+ const requiredFields = ['REDDIT_CLIENT_ID', 'REDDIT_CLIENT_SECRET', 'REDDIT_REFRESH_TOKEN', 'REDDIT_USERNAME'];
+ const missingFields = requiredFields.filter(field => !encryptedAccount[field]);
+ if (missingFields.length > 0) {
+ throw new Error(`Missing required fields: ${missingFields.join(', ')}`);
+ }
+
// Decrypt credentials
const credentials = {📝 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.
| for (let i = 0; i < encryptedAccounts.length; i++) { | |
| const encryptedAccount = encryptedAccounts[i]; | |
| try { | |
| // Decrypt credentials | |
| const credentials = { | |
| REDDIT_CLIENT_ID: decrypt(encryptedAccount.REDDIT_CLIENT_ID), | |
| REDDIT_CLIENT_SECRET: decrypt(encryptedAccount.REDDIT_CLIENT_SECRET), | |
| REDDIT_REFRESH_TOKEN: decrypt(encryptedAccount.REDDIT_REFRESH_TOKEN), | |
| REDDIT_USERNAME: decrypt(encryptedAccount.REDDIT_USERNAME) | |
| }; | |
| // Generate account ID from username (for uniqueness) | |
| const accountId = `reddit_${credentials.REDDIT_USERNAME}`; | |
| // Get usage statistics from Redis | |
| const usage = await this.getApiKeyUsageLocal(accountId); | |
| accounts.push({ | |
| accountId, | |
| credentials, | |
| lastUsed: usage.last_request || undefined, | |
| totalRequests: usage.total_requests | |
| }); | |
| } catch (e) { | |
| console.warn(`Failed to decrypt Reddit account ${i + 1}:`, e); | |
| continue; | |
| } | |
| for (let i = 0; i < encryptedAccounts.length; i++) { | |
| const encryptedAccount = encryptedAccounts[i]; | |
| try { | |
| // Validate required fields exist | |
| const requiredFields = ['REDDIT_CLIENT_ID', 'REDDIT_CLIENT_SECRET', 'REDDIT_REFRESH_TOKEN', 'REDDIT_USERNAME']; | |
| const missingFields = requiredFields.filter(field => !encryptedAccount[field]); | |
| if (missingFields.length > 0) { | |
| throw new Error(`Missing required fields: ${missingFields.join(', ')}`); | |
| } | |
| // Decrypt credentials | |
| const credentials = { | |
| REDDIT_CLIENT_ID: decrypt(encryptedAccount.REDDIT_CLIENT_ID), | |
| REDDIT_CLIENT_SECRET: decrypt(encryptedAccount.REDDIT_CLIENT_SECRET), | |
| REDDIT_REFRESH_TOKEN: decrypt(encryptedAccount.REDDIT_REFRESH_TOKEN), | |
| REDDIT_USERNAME: decrypt(encryptedAccount.REDDIT_USERNAME) | |
| }; | |
| // Generate account ID from username (for uniqueness) | |
| const accountId = `reddit_${credentials.REDDIT_USERNAME}`; | |
| // Get usage statistics from Redis | |
| const usage = await this.getApiKeyUsageLocal(accountId); | |
| accounts.push({ | |
| accountId, | |
| credentials, | |
| lastUsed: usage.last_request || undefined, | |
| totalRequests: usage.total_requests | |
| }); | |
| } catch (e) { | |
| console.warn(`Failed to decrypt Reddit account ${i + 1}:`, e); | |
| continue; | |
| } | |
| } |
| private async getApiKeyUsageLocal( | ||
| accountId: string | ||
| ): Promise<{ total_requests: number; last_request: string | null }> { | ||
| await this.ensureConnected(); | ||
| const key = `reddit_accounts:${accountId}`; | ||
| const data = await this.redisClient.hGetAll(key); | ||
| return { | ||
| total_requests: data?.total_requests ? parseInt(data.total_requests, 10) : 0, | ||
| last_request: data?.last_request ?? null | ||
| }; | ||
| } |
There was a problem hiding this comment.
Use this.usageKeyPrefix instead of hardcoded string.
Line 92 hardcodes 'reddit_accounts' when constructing the Redis key, but the class defines this.usageKeyPrefix = 'reddit_accounts' on line 22. This creates inconsistency and makes the code harder to maintain if the prefix ever needs to change.
Apply this diff to use the class property:
private async getApiKeyUsageLocal(
accountId: string
): Promise<{ total_requests: number; last_request: string | null }> {
await this.ensureConnected();
- const key = `reddit_accounts:${accountId}`;
+ const key = `${this.usageKeyPrefix}:${accountId}`;
const data = await this.redisClient.hGetAll(key);🤖 Prompt for AI Agents
In src/services/redditAccountManager.ts around lines 88 to 98, replace the
hardcoded Redis key prefix 'reddit_accounts' with the class property
this.usageKeyPrefix when building the key variable; ensure the key is
constructed as `${this.usageKeyPrefix}:${accountId}` (or equivalent string
concat) so it uses the configured prefix consistently, leaving the rest of the
function unchanged.
| protected async trackApiKeyUsageLocal(accountId: string): Promise<void> { | ||
| await this.ensureConnected(); | ||
| const key = `reddit_accounts:${accountId}`; | ||
| const now = new Date().toISOString(); | ||
| await this.redisClient | ||
| .multi() | ||
| .hIncrBy(key, 'total_requests', 1) | ||
| .hSet(key, { last_request: now, account_id: accountId }) | ||
| .exec(); | ||
| } |
There was a problem hiding this comment.
Use this.usageKeyPrefix instead of hardcoded string.
Line 102 has the same issue as line 92: it hardcodes 'reddit_accounts' instead of using this.usageKeyPrefix.
Apply this diff:
protected async trackApiKeyUsageLocal(accountId: string): Promise<void> {
await this.ensureConnected();
- const key = `reddit_accounts:${accountId}`;
+ const key = `${this.usageKeyPrefix}:${accountId}`;
const now = new Date().toISOString();📝 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.
| protected async trackApiKeyUsageLocal(accountId: string): Promise<void> { | |
| await this.ensureConnected(); | |
| const key = `reddit_accounts:${accountId}`; | |
| const now = new Date().toISOString(); | |
| await this.redisClient | |
| .multi() | |
| .hIncrBy(key, 'total_requests', 1) | |
| .hSet(key, { last_request: now, account_id: accountId }) | |
| .exec(); | |
| } | |
| protected async trackApiKeyUsageLocal(accountId: string): Promise<void> { | |
| await this.ensureConnected(); | |
| const key = `${this.usageKeyPrefix}:${accountId}`; | |
| const now = new Date().toISOString(); | |
| await this.redisClient | |
| .multi() | |
| .hIncrBy(key, 'total_requests', 1) | |
| .hSet(key, { last_request: now, account_id: accountId }) | |
| .exec(); | |
| } |
🤖 Prompt for AI Agents
In src/services/redditAccountManager.ts around lines 100 to 109, the code builds
the Redis key using a hardcoded 'reddit_accounts' string; replace that with
this.usageKeyPrefix so the key becomes `${this.usageKeyPrefix}:${accountId}`
(preserving the colon and accountId), ensuring consistency with line 92; keep
the rest of the logic (ensureConnected, hIncrBy, hSet, exec) unchanged.
A new file
redditAccountManager.tshas been added in services with a Reddit account manager implementationSummary by CodeRabbit