fix: data plane events buffer on load option flag #1997
Conversation
📝 Walkthrough## Walkthrough
The pull request introduces refactoring in two key analytics-related classes: `Analytics` and `EventRepository`. In the `Analytics` class, a new private method `setActiveDestinations` is added to encapsulate the logic for setting active destinations, replacing inline code. Similarly, in the `EventRepository` class, a new private method `startDpEventsQueue` is introduced to manage the data plane events queue initialization, consolidating and simplifying the event buffering and queue start conditions.
## Changes
| File | Change Summary |
|------|----------------|
| `packages/analytics-js/src/components/core/Analytics.ts` | Added private `setActiveDestinations()` method to centralize logic for setting active destinations |
| `packages/analytics-js/src/components/eventRepository/EventRepository.ts` | Added private `startDpEventsQueue()` method to manage data plane events queue initialization and buffering logic |
## Possibly related PRs
- #2100: Refactors internal logic of starting the data plane events queue in the `EventRepository` class, closely related to buffering and queue management improvements.
## Suggested reviewers
- koladilipTip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1997 +/- ##
===========================================
+ Coverage 61.61% 61.62% +0.01%
===========================================
Files 478 478
Lines 16450 16455 +5
Branches 3265 3267 +2
===========================================
+ Hits 10136 10141 +5
+ Misses 5084 5066 -18
- Partials 1230 1248 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/analytics-js/src/components/eventRepository/EventRepository.ts (1)
125-130: Consider a more descriptive variable name.The variable
shouldBufferDpEventscould be more descriptive to better reflect its specific purpose.- const shouldBufferDpEvents = bufferEventsUntilReady && hybridDestExist; + const shouldBufferForHybridDestinations = bufferEventsUntilReady && hybridDestExist;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/analytics-js/src/components/core/Analytics.ts(2 hunks)packages/analytics-js/src/components/eventRepository/EventRepository.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/analytics-js/src/components/eventRepository/EventRepository.ts (3)
119-121: LGTM! Appropriate condition for starting data plane events queue.The condition correctly initiates the data plane events queue when pre-consent buffering is not required.
Line range hint
124-152: LGTM! Well-structured queue management implementation.The method effectively consolidates the data plane events queue management logic with appropriate buffering conditions and safety timeout mechanism.
156-164: LGTM! Consistent use of centralized queue management.The resume method correctly uses the new startDpEventsQueue method while maintaining existing queue clearing logic.
packages/analytics-js/src/components/core/Analytics.ts (2)
294-296: LGTM! Good extraction of active destinations setup.The code organization is improved by moving the active destinations setup to a dedicated method.
304-312: LGTM! Well-encapsulated active destinations setup.The new method effectively encapsulates the plugin invocation logic for setting active destinations, promoting code reuse and maintainability.
size-limit report 📦
|
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
|
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
saikumarrs
force-pushed
the
fix.bufferDpEvents-sdk-2862
branch
from
04c5fa8 to
663d9cd
Compare
|
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
PR Description
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
https://linear.app/rudderstack/issue/SDK-2862/buffer-data-plane-events-until-ready
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit