-
-
Notifications
You must be signed in to change notification settings - Fork 11k
🐛 Fix: Referral banner not shown on direct navigation to non-dashboard routes #24449
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
base: main
Are you sure you want to change the base?
🐛 Fix: Referral banner not shown on direct navigation to non-dashboard routes #24449
Conversation
Warning Rate limit exceeded@niranjan-uma-shankar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce a new utility function, Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes The changes primarily involve refactoring existing logic to use a new utility function, adding service injection, fixing a small typo, and deleting a component and its template. The scope is limited to a few files with medium-complexity changes, requiring moderate attention but no extensive cross-cutting modifications. Possibly related issues
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
@@ -53,7 +55,24 @@ export default class FooterBanner extends Component { | |||
|
|||
@task | |||
*loadCurrentMRR() { | |||
if (this.isAdminOrOwnern) { | |||
if (this.isAdminOrOwner) { |
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.
Corrects typo
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: 0
♻️ Duplicate comments (1)
ghost/admin/app/components/gh-nav-menu/footer-banner.js (1)
58-58
: Critical fix: Corrected typo in admin role check.This typo fix from
isAdminOrOwnern
toisAdminOrOwner
addresses the root cause described in the PR objectives. The banner was not showing on non-dashboard routes because this typo prevented the MRR loading logic from executing.
🧹 Nitpick comments (1)
ghost/admin/app/utils/load-dashboard-mock-state.js (1)
11-14
: Consider consolidating redundant null checks.The current implementation checks
if (parsed)
afterJSON.parse()
, butJSON.parse()
of a valid JSON string will never return a falsy value unless the stored value was explicitlynull
,false
,0
, or""
. Consider simplifying the logic or adding more specific validation.- if (savedState) { - const parsed = JSON.parse(savedState); - if (parsed) { - state.savedState = parsed; - } - } + if (savedState) { + state.savedState = JSON.parse(savedState); + }Also applies to: 19-22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/admin/app/components/dashboard/prototype/control-panel.js
(2 hunks)ghost/admin/app/components/gh-nav-menu/footer-banner.js
(2 hunks)ghost/admin/app/utils/load-dashboard-mock-state.js
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
ghost/admin/app/components/gh-nav-menu/footer-banner.js (1)
Learnt from: kevinansfield
PR: TryGhost/Ghost#23770
File: apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModalLabs.tsx:435-450
Timestamp: 2025-06-10T11:07:10.800Z
Learning: In Ghost's newsletter customization features, when promoting a feature from alpha to beta status, the feature flag guards are updated to make the feature available under both the `emailCustomization` (beta) and `emailCustomizationAlpha` (alpha) flags. This is done by either removing conditional guards entirely when the component is already behind both flags, or by updating conditionals to check for either flag.
🧬 Code Graph Analysis (2)
ghost/admin/app/components/dashboard/prototype/control-panel.js (1)
ghost/admin/app/utils/load-dashboard-mock-state.js (4)
loadDashboardMockState
(1-37)savedState
(9-9)savedStatus
(17-17)enabledStr
(25-25)
ghost/admin/app/components/gh-nav-menu/footer-banner.js (1)
ghost/admin/app/utils/load-dashboard-mock-state.js (4)
loadDashboardMockState
(1-37)savedState
(9-9)savedStatus
(17-17)enabledStr
(25-25)
🔇 Additional comments (7)
ghost/admin/app/utils/load-dashboard-mock-state.js (2)
1-37
: LGTM: Well-structured utility function with proper error handling.The function effectively centralizes dashboard mock state loading logic and handles edge cases appropriately. The error handling for localStorage unavailability is particularly good.
28-30
: Good type validation for the boolean flag.The explicit
typeof parsed === 'boolean'
check ensures type safety for the enabled flag, which is important since it controls feature behavior.ghost/admin/app/components/dashboard/prototype/control-panel.js (2)
3-3
: Good refactor: Centralizing localStorage logic.The import of the new utility function is appropriate and helps eliminate code duplication.
75-89
: Excellent simplification of the loadState method.The refactor successfully removes inline localStorage access and JSON parsing, making the code more maintainable and reducing duplication. The conditional assignments are clean and preserve the original behavior.
ghost/admin/app/components/gh-nav-menu/footer-banner.js (3)
5-5
: Good addition of required imports and services.The import of
loadDashboardMockState
utility and injection ofdashboardMocks
service are necessary for the mock state functionality.Also applies to: 12-12
59-74
: Well-implemented mock state loading for non-production environments.The logic correctly applies mock dashboard state when not in production, ensuring consistent behavior across all routes. The implementation mirrors the pattern used in the control panel component, maintaining consistency across the codebase.
60-74
: Mock loading is correctly limited to non-production buildsThe
envConfig.environment
value inghost/admin/config/environment.js
can only be"development"
,"test"
, or"production"
. Since your code uses:if (envConfig.environment !== 'production') { // load mocks… }the mock state will only be applied in development and test builds—not in production. No additional changes are required.
Thank you @niranjan-uma-shankar for this PR. Your change looks good. Could you please add some tests for this? |
ca2d5db
to
85f592f
Compare
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: 0
🧹 Nitpick comments (1)
ghost/admin/tests/acceptance/footer-banner-referral-invite-test.js (1)
1-13
: Well-structured test setup with comprehensive imports.The test setup follows Ember testing best practices with proper hooks and imports. However,
moment
is imported but doesn't appear to be used in the tests.Consider removing the unused import:
-import moment from 'moment-timezone';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/admin/tests/acceptance/footer-banner-referral-invite-test.js
(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sagzy
PR: TryGhost/Ghost#24346
File: apps/admin-x-settings/src/components/settings/growth/Network.tsx:8-12
Timestamp: 2025-07-14T12:20:35.268Z
Learning: The Network component toggle in `apps/admin-x-settings/src/components/settings/growth/Network.tsx` is intentionally implemented as static UI with a no-op onChange handler, as part of a UI-first development approach before connecting actual ActivityPub functionality.
ghost/admin/tests/acceptance/footer-banner-referral-invite-test.js (4)
Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file apps/admin-x-activitypub/test/unit/utils/pending-activity.ts
.
Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in apps/admin-x-activitypub/test/unit/utils/pending-activity.ts
, which covers generatePendingActivityId
, isPendingActivity
, and generatePendingActivity
functions.
Learnt from: ErisDS
PR: #23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response body
field is empty while the actual CSV data is provided through the text
field. Tests should use expectEmptyBody()
and then validate the CSV content via .expect(({text}) => ...)
- this is not contradictory behavior.
Learnt from: ErisDS
PR: #23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
🔇 Additional comments (3)
ghost/admin/tests/acceptance/footer-banner-referral-invite-test.js (3)
15-33
: Excellent helper function for test data generation.The
mrrResponseObject
helper creates well-structured mock MRR responses that match the expected API format. This abstraction makes the tests more maintainable and readable.
35-99
: Comprehensive test coverage for Administrator role scenarios.The test suite thoroughly covers all the key scenarios for the referral banner functionality:
- Direct navigation fix verification: Tests confirm the banner appears on both
/pages
and/dashboard
routes, directly addressing the bug described in the PR objectives- Dismissal persistence: Properly tests that dismissed banners stay dismissed across route changes
- Feature service integration: Correctly verifies the
referralInviteDismissed
state is set- Conditional display logic: Tests both Stripe enablement and MRR threshold requirements
The MRR values (50000 = $500, 7500 = $75) correctly test above and below the $100 threshold.
101-117
: Appropriate test coverage for Editor role restrictions.The test correctly verifies that Editors don't see the referral banner even when all other conditions are met (Stripe enabled, MRR > $100). This properly tests the role-based access control for the referral feature.
fixes TryGhost#24448 This file extracts the logic to load saved mock metadata, so that it can be reused in the footer banner component.
fixes TryGhost#24448 To improve reusability and reduce duplication across modules.
ref TryGhost#24448 Previously, the catch block returned an empty object, which could break consumers expecting a consistent state shape. This change ensures a stable return value even if localStorage is unavailable or contains malformed JSON.
ref TryGhost#24448 Included mock state loading in loadCurrentMRR to ensure MRR data is available, which is required to determine eligibility for showing the referral invite banner. This guarantees consistent initialization of MRR data and mock settings, regardless of the initial route.
ref TryGhost#24448 The files `gh-referral-invite.hbs` and `gh-referral-invite.js` under `ghost/admin/app/components/` are no longer referenced anywhere in the codebase. This component appears to be obsolete and can be safely removed to reduce dead code and improve maintainability. The components were originally introduced in TryGhost#16187.
To ensure that the referral invite banner behaves correctly when an account is eligible to be shown the banner.
40acb96
to
a9c8136
Compare
Uses object property shorthand for improved readability.
Moment is not used in the file.
This PR fixes a bug that prevented the referral invite banner from appearing when users directly navigated to non-dashboard routes such as
/pages
or/posts
. Since some users may have these routes bookmarked, it's important that the banner shows consistently across all eligible pages.The issue is tracked in #24448.
Root Cause
A typo in the role check (isAdminOrOwnern) prevented the API call to fetch MRR data from being made on non-dashboard routes. Without MRR data, the banner's visibility condition was never satisfied.
Fixes
/dashboard
.This makes the banner behaviour more predictable and helps developers reliably test referral eligibility with mock data.
gh1.mp4
History
The referral banner was first implemented in #16187, and later copied over to the "What's new" banner implementation in #20112. The typo exists in both implementations.
Testing Instructions
/dashboard
and confirm that the referral banner is visible in the nav menu./pages
or/posts
.Now:
-You should now see an outgoing request to
/stats/mrr
in the Network tab.This confirms that MRR data is being fetched even when directly visiting non-dashboard routes, enabling the referral banner to appear for eligible accounts.
ghost/admin/tests
folder, execute the following and verify that the tests pass: