feat: pushBanner flags per platform#3955
Conversation
It was misleading since it also handles promoting mobile application I also set this config to true again
doubleface
requested review from
JF-Cozy,
lethemanh,
rezk2ll and
zatteo
as code owners
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR renames the configuration key Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
BundleMonFiles updated (1)
Unchanged files (19)
Total files change +23B 0% Groups updated (1)
Unchanged groups (2)
Final result: ✅ View report in BundleMon website ➡️ |
doubleface
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/flags.js (1)
3-17: 🎯 Functional Correctness | 🔴 CriticalProduction activation path for new banner flags is broken.
In production,
initFlags()only callsflagsList()ifactivateFlagsis true (lines 15–17). However,activateFlagsis false by default in production unless theswitcherflag is already explicitly set or the?flagsURL parameter is provided. This creates a bootstrap problem: the new flags at lines 27–28 are never initialized unless switcher is pre-enabled externally or a developer appends?flagsto the URL.The
flag('switcher', true)call at line 22 is unreachable in the common case, leaving no way for the switcher to be activated without external intervention. For runtime control of the push banner feature without deployment, ensure theswitcherflag is initialized from the server, or update the bootstrap logic to callflagsList()unconditionally on first load.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/flags.js` around lines 3 - 17, The `initFlags()` function only calls `flagsList()` when `activateFlags` is true, but in production `activateFlags` defaults to false unless the switcher flag is already set or the ?flags URL parameter is provided. This creates a bootstrap problem where the flags are never initialized. To fix this, ensure that `flagsList()` is called unconditionally on first load to initialize all flags regardless of the `activateFlags` condition, or alternatively ensure the switcher flag is pre-initialized from the server before `initFlags()` executes. Update the conditional logic at the end of `initFlags()` to guarantee `flagsList()` is invoked to properly bootstrap the flag system.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/flags.js`:
- Around line 3-17: The `initFlags()` function only calls `flagsList()` when
`activateFlags` is true, but in production `activateFlags` defaults to false
unless the switcher flag is already set or the ?flags URL parameter is provided.
This creates a bootstrap problem where the flags are never initialized. To fix
this, ensure that `flagsList()` is called unconditionally on first load to
initialize all flags regardless of the `activateFlags` condition, or
alternatively ensure the switcher flag is pre-initialized from the server before
`initFlags()` executes. Update the conditional logic at the end of `initFlags()`
to guarantee `flagsList()` is invoked to properly bootstrap the flag system.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3407bfaf-04fb-40ee-be70-e8ef81b209d1
📒 Files selected for processing (6)
src/components/pushClient/Banner.jsxsrc/components/pushClient/Button.jsxsrc/config/config.jsonsrc/lib/flags.jssrc/modules/viewer/CallToAction.jsxsrc/modules/viewer/CallToAction.spec.jsx
doubleface
force-pushed
the
feat/pushBannerAppFlag
branch
from
85f67f6 to
195b6dd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ide-desktop.enabled To be able to hide one banner or another as we want in production
doubleface
force-pushed
the
feat/pushBannerAppFlag
branch
from
aa8d075 to
edaeea5
Compare
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
2 participants
What
Rename
promoteDesktopconfig topromoteAppand add per-platform feature flags (drive.pushBanner-hide-mobile.enabled/drive.pushBanner-hide-desktop.enabled) to control which push-to-install banner is shown.And hide mobile application push banner by defaut
Why
The
promoteDesktopconfig name was misleading — it actually controls both desktop and mobile app promotion banners in the push client. The rename makes the intent clearer.Additionally, we wanted the ability to independently toggle the mobile vs desktop push banner in production without a deploy, hence the new
cozy-flags.Summary by CodeRabbit
Summary
New Features
Chores