chore: beta deployment for dynamic destination configuration#2278
chore: beta deployment for dynamic destination configuration#2278saikumarrs wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe change updates the logic in the device mode destinations module to ensure that disabled destinations are filtered out earlier in the selection process. It achieves this by applying the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DeviceModeDestinations
Caller->>DeviceModeDestinations: Get destinations (with/without overrides)
alt Overrides exist
DeviceModeDestinations->>DeviceModeDestinations: Use overridden destinations
else No overrides
DeviceModeDestinations->>DeviceModeDestinations: Apply filterDisabledDestination to supported destinations
end
DeviceModeDestinations-->>Caller: Return filtered destination list
Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR alters the beta deployment workflow to bypass mergeability checks by replacing the original prDetails.mergeable and prDetails.mergeable_state conditions with hardcoded false.
- Disabled the check for
prDetails.mergeable === false - Disabled the check for
prDetails.mergeable_state !== 'clean'
Comments suppressed due to low confidence (2)
.github/workflows/deploy-beta.yml:82
- Replacing the mergeable check with
if (false)disables conflict detection, allowing non-mergeable PRs to deploy. Restore the originalif (prDetails.mergeable === false)condition.
if (false) {
.github/workflows/deploy-beta.yml:89
- Replacing the clean-state check with
if (false)bypasses the requirement that the PR state be 'clean', risking deployments of unapproved or failing PRs. Restoreif (prDetails.mergeable_state !== 'clean').
if (false) {
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2278 +/- ##
========================================
Coverage 62.09% 62.09%
========================================
Files 479 479
Lines 16621 16621
Branches 3284 3280 -4
========================================
Hits 10321 10321
- Misses 5092 5099 +7
+ Partials 1208 1201 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/analytics-js-plugins/src/deviceModeDestinations/index.ts (1)
57-64: Early filtering is correct but please verify naming and duplication.Applying
filterDisabledDestinationbefore any consent / load-only filtering closes the loophole where destinations withenabled: falsecould still be loaded, so the behavioural change is 👍.Two minor follow-ups to consider:
- : filterDisabledDestination(configSupportedDestinations); + : filterDisabledDestination(configSupportedDestinations);
The helper’s singular name (
filterDisabledDestination) is slightly misleading because it processes an array. Renaming tofilterDisabledDestinationswould better communicate intent.We now filter disabled destinations twice: here and later through
filterDestinations, which already checks integration settings. IffilterDisabledDestinationandfilterDestinationsshare logic, consolidating them would avoid duplicate iterations and reduce cognitive load.Neither point is blocking, but addressing them would tighten the API surface.
Please confirm that
filterDestinationsdoes not already excludeenabled: falsedestinations to prevent redundant work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/deploy-beta.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
packages/analytics-js-plugins/src/deviceModeDestinations/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Bundle size checks
🔇 Additional comments (1)
packages/analytics-js-plugins/src/deviceModeDestinations/index.ts (1)
16-17: Import looks good – no further action.
filterDisabledDestinationis now explicitly imported alongside the other helpers and is used below, so there are no dead-code or tree-shaking concerns here.
|
|
The feature has been released to production so this is not required anymore. |
PR Description
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit