-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Valentin/Catalyst upgrades #7996
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
Conversation
WalkthroughThis update refines experiment logic and styling for the Catalyst feature. It adjusts probability thresholds and experiment checks in tracking and user experiment assignment, clarifies control flow in experiment value determination, centralizes styling variables for Catalyst portal layout, and prevents anonymous signup prompts for Catalyst experiment users. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CampaignView
participant FullStoryTracker
User->User: getCatalystExperimentValue()
alt Experiment inactive
User-->Caller: 'control'
else Experiment active
User-->Caller: 'beta' or 'control' (probabilistic)
end
FullStoryTracker->User: getCatalystExperimentValue()
FullStoryTracker->FullStoryTracker: decideEnabled() (probabilistic based on experiment value)
CampaignView->User: isCatalyst (via getCatalystExperimentValue)
CampaignView->CampaignView: shouldShow()
alt User is in Catalyst
CampaignView-->UI: Do not show anonymous classroom signup
else Not in Catalyst
CampaignView-->UI: Show prompt if other conditions met
end
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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. 🪧 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 (
|
…t portal dimensions and adjusting margins for improved layout consistency.
… condition checks in FullStoryTracker
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/core/Tracker2/FullStoryTracker.js
(1 hunks)app/core/utils.js
(2 hunks)app/locale/en.js
(1 hunks)app/models/User.js
(1 hunks)app/styles/play/campaign-view.sass
(4 hunks)app/views/play/CampaignView.js
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint CI
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (10)
app/models/User.js (1)
1336-1340
: Control flow improvement for better clarity and efficiencyThe refactored code now immediately returns 'control' when the experiment probability is null instead of continuing with unnecessary execution. This is a cleaner approach that prevents the code from running unneeded probability calculations.
app/locale/en.js (1)
4985-4986
: Swap of season labels aligns with updated arena metadata.The updated values for
season_14
("Racing Rivals") andseason_15
("Soccer Showdown") correctly mirror the corresponding changes in the season configuration and scheduling. This ensures that the displayed season names remain consistent across the UI and underlying logic.app/views/play/CampaignView.js (1)
2200-2201
: Suppress classroom signup dialog for Catalyst experiment usersThe condition
!this.isCatalyst
is added to prevent showing the anonymous classroom signup dialog to users who are part of the Catalyst experiment. This aligns with the PR objective of removing the teacher pop-up to test its impact.app/core/Tracker2/FullStoryTracker.js (3)
109-109
: Reduced tracking probability for anonymous usersThe probability threshold for enabling FullStory tracking for anonymous users has been lowered from 0.25% to 0.15%, reducing the overall tracking footprint.
112-114
: Updated experiment name from M7 to CatalystThe code now checks for
getCatalystExperimentValue()
instead ofgetM7ExperimentValue()
while maintaining the same 5% probability threshold. The log message has been appropriately updated to reflect this change.
115-115
: Reduced tracking probability for non-parent teachersThe probability threshold for enabling FullStory tracking for teachers who are not parents has been reduced from 2% to 1%.
app/styles/play/campaign-view.sass (3)
9-11
: Added variables for Catalyst portal layoutThese new SASS variables improve maintainability by centralizing layout constants for the Catalyst portal. This makes future adjustments easier and ensures consistent values across the stylesheet.
1375-1375
: Replaced hardcoded margin values with variablesUsing the
$catalystMarginTopBase
variable with appropriate offsets improves code maintainability and makes the relationship between different margin values clearer.Also applies to: 1383-1383, 1386-1386
1455-1457
: Replaced hardcoded height values with variablesUsing the
$sideCampaignPortalHeight
variable instead of hardcoded pixel values improves consistency and makes it easier to adjust the layout proportionally if needed in the future.Also applies to: 1472-1472
app/core/utils.js (1)
1383-1384
: AILeagueSeasons configuration updated to match arena changesThe championshipType and image properties for seasons 14 and 15 have been swapped to match the arena changes:
- Season 14: now 'clash' type with 'grand-prix-clash' image
- Season 15: now 'blitz' type with 'golden-goal-blitz' image
This change aligns with the arena slug changes in the arenas array.
Summary by CodeRabbit
Bug Fixes
Style
Refactor