Feature/optimize event buffer #278
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces local micro-batching to the event buffer with controlled Redis flushing, removes redundant Redis keyevent subscription logic from the live controller, and adds a new interactive script for assigning Polar products to organizations. Events are now buffered in memory and flushed in batches rather than written per-event. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In `@packages/db/src/buffers/event-buffer.ts`:
- Around line 395-402: The catch block in the flushLocalBufferToRedis flow
currently logs the failure but drops events from eventsToFlush (causing data
loss); modify the catch handler in the method that builds and executes the Redis
multi (look for eventsToFlush and this.pendingEvents) to re-queue the failed
events back onto this.pendingEvents (preserving order—e.g., unshift or concat
depending on how pendingEvents is consumed), ensure you do this before
clearing/resetting isFlushing, and avoid duplicating events if a partial success
path exists (check any success branch that removes events). Also consider
incrementing a retry counter or emitting a metric/log entry when re-queuing so
persistent failures are observable.
In `@packages/payments/scripts/assign-product-to-org.ts`:
- Around line 42-50: The prompt object for the Polar API key uses the wrong
Inquirer type string ('string'); update the prompt definition for polarApiKey so
its type property is 'input' (i.e., change type: 'string' to type: 'input')
while keeping the existing name 'polarApiKey' and validate function intact to
ensure the same validation behavior.
🧹 Nitpick comments (3)
packages/payments/scripts/assign-product-to-org.ts (1)
130-133: Avoid duplicate Polar client instantiation.A Polar client is already created at line 54-57 inside
promptForInput()but is not returned. Instead of creating it again here, consider returning the client frompromptForInput()or moving client creation tomain()only.Option: Return polar client from promptForInput
- return { - ...polarCredentials, - ...restOfAnswers, - }; + return { + ...polarCredentials, + ...restOfAnswers, + polar, // Return the already-created client + }; } async function main() { console.log('Assigning existing product to organization...'); const input = await promptForInput(); - const polar = new Polar({ - accessToken: input.polarApiKey, - server: input.isProduction ? 'production' : 'sandbox', - }); + const polar = input.polar;packages/db/src/buffers/event-buffer.ts (1)
60-71: Consider cleaning up timers on shutdown.The
flushTimerandpublishTimerare not cleaned up if the EventBuffer is destroyed or the process shuts down gracefully. This could lead to timer leaks or callbacks firing after cleanup.Consider adding a cleanup/destroy method
public destroy() { if (this.flushTimer) { clearTimeout(this.flushTimer); this.flushTimer = null; } if (this.publishTimer) { clearTimeout(this.publishTimer); this.publishTimer = null; } }packages/db/src/buffers/event-buffer.test.ts (1)
83-157: Consider verifying calculated duration in the test.The test validates that events move to the buffer correctly but doesn't verify that the duration is actually calculated. Consider adding a check that processes the buffer and verifies the duration field is set correctly (e.g., ~1000ms between view1 and view2).
| } catch (error) { | ||
| this.logger.error('Failed to add event to Redis buffer', { error }); | ||
| this.logger.error('Failed to flush local buffer to Redis', { | ||
| error, | ||
| eventCount: eventsToFlush.length, | ||
| }); | ||
| } finally { | ||
| this.isFlushing = false; | ||
| } |
There was a problem hiding this comment.
Events may be lost on Redis flush failure.
If multi.exec() fails, the events in eventsToFlush are discarded without retry. For critical event tracking, consider re-queuing failed events back to pendingEvents in the catch block.
Proposed fix to re-queue failed events
} catch (error) {
this.logger.error('Failed to flush local buffer to Redis', {
error,
eventCount: eventsToFlush.length,
});
+ // Re-queue failed events for retry
+ this.pendingEvents = [...eventsToFlush, ...this.pendingEvents];
} finally {
this.isFlushing = false;
}📝 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.
| } catch (error) { | |
| this.logger.error('Failed to add event to Redis buffer', { error }); | |
| this.logger.error('Failed to flush local buffer to Redis', { | |
| error, | |
| eventCount: eventsToFlush.length, | |
| }); | |
| } finally { | |
| this.isFlushing = false; | |
| } | |
| } catch (error) { | |
| this.logger.error('Failed to flush local buffer to Redis', { | |
| error, | |
| eventCount: eventsToFlush.length, | |
| }); | |
| // Re-queue failed events for retry | |
| this.pendingEvents = [...eventsToFlush, ...this.pendingEvents]; | |
| } finally { | |
| this.isFlushing = false; | |
| } |
🤖 Prompt for AI Agents
In `@packages/db/src/buffers/event-buffer.ts` around lines 395 - 402, The catch
block in the flushLocalBufferToRedis flow currently logs the failure but drops
events from eventsToFlush (causing data loss); modify the catch handler in the
method that builds and executes the Redis multi (look for eventsToFlush and
this.pendingEvents) to re-queue the failed events back onto this.pendingEvents
(preserving order—e.g., unshift or concat depending on how pendingEvents is
consumed), ensure you do this before clearing/resetting isFlushing, and avoid
duplicating events if a partial success path exists (check any success branch
that removes events). Also consider incrementing a retry counter or emitting a
metric/log entry when re-queuing so persistent failures are observable.
| { | ||
| type: 'string', | ||
| name: 'polarApiKey', | ||
| message: 'Enter your Polar API key:', | ||
| validate: (input: string) => { | ||
| if (!input) return 'API key is required'; | ||
| return true; | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Use 'input' instead of 'string' for inquirer prompt type.
Inquirer's prompt type should be 'input' for text input fields, not 'string'.
Proposed fix
{
- type: 'string',
+ type: 'input',
name: 'polarApiKey',
message: 'Enter your Polar API key:',
validate: (input: string) => {
if (!input) return 'API key is required';
return true;
},
},📝 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.
| { | |
| type: 'string', | |
| name: 'polarApiKey', | |
| message: 'Enter your Polar API key:', | |
| validate: (input: string) => { | |
| if (!input) return 'API key is required'; | |
| return true; | |
| }, | |
| }, | |
| { | |
| type: 'input', | |
| name: 'polarApiKey', | |
| message: 'Enter your Polar API key:', | |
| validate: (input: string) => { | |
| if (!input) return 'API key is required'; | |
| return true; | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In `@packages/payments/scripts/assign-product-to-org.ts` around lines 42 - 50, The
prompt object for the Polar API key uses the wrong Inquirer type string
('string'); update the prompt definition for polarApiKey so its type property is
'input' (i.e., change type: 'string' to type: 'input') while keeping the
existing name 'polarApiKey' and validate function intact to ensure the same
validation behavior.
Summary by CodeRabbit
New Features
Performance
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.