Added Feedback submission widget#25801
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Featurebase feedback support across admin and core: introduces a new admin API module and public client to fetch Featurebase tokens, a React hook to load and initialize the Featurebase SDK with a JWT, a conditional Feedback menu item and mobile-aware NavMenuButton in the admin UI, a backend token endpoint that returns signed JWTs, serializers and routes for the endpoint, exposes featurebase in public config when the labs flag is enabled, adds a private labs flag, and includes unit and end-to-end tests for the new behavior. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20793801179 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
kevinansfield
force-pushed
the
featurebase
branch
3 times, most recently
from
a0bfeda to
4e403c6
Compare
- Added explicit type aliases for cleaner TypeScript definitions - Removed unused `isReady` return value (ref-based state was never consumed) - Changed script detection from getElementById to querySelector by URL - Added `site` to useEffect dependencies to ensure data availability - Simplified async/await IIFE to promise .then() chain - Removed unnecessary callback handling for widgetReady action
followed pattern for `/tinybird/token/`
kevinansfield
force-pushed
the
featurebase
branch
from
d0e9265 to
4b85a06
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/admin/src/hooks/use-featurebase.ts:
- Around line 19-38: The loadFeaturebaseSDK Promise never rejects on
network/script load failures; update the loadFeaturebaseSDK function to use new
Promise((resolve, reject)) and add script.onerror = () => reject(new
Error(`Failed to load Featurebase SDK from ${SDK_URL}`)); so the caller's .catch
runs; keep the existing document.querySelector check and script.onload behavior,
and ensure the temporary window.Featurebase queue setup remains unchanged.
In @ghost/core/core/server/api/endpoints/featurebase.js:
- Around line 20-22: The JWT created in the jwt.sign call inside featurebase.js
is missing an expiration; update the jwt.sign invocation to include an expiresIn
option (e.g. { algorithm: 'HS256', expiresIn: <value> }) and source the expiry
from configuration (add/read e.g. config.get('featurebase:jwtExpiry') with a
sensible default like '1h') so tokens automatically expire; ensure any config
schema or consumers are updated to accept the new config key.
🧹 Nitpick comments (3)
ghost/core/test/unit/server/services/public-config/config.test.js (1)
141-155: Consider adding explicit assertion for sensitive field exclusion.Test 2 configures
jwtSecretbut doesn't explicitly verify it's excluded from the response. While test 3 covers this scenario comprehensively, adding an assertion here would make this test self-contained.🔧 Suggested improvement
assert.equal(configProperties.featurebase.enabled, false); assert.equal(configProperties.featurebase.organization, 'test-org'); + assert.equal(configProperties.featurebase.jwtSecret, undefined); });ghost/core/test/e2e-api/admin/featurebase.test.js (1)
48-49: Consider movingrequire('jsonwebtoken')to the top of the file.Inline requires work but placing the import at the file top with other dependencies improves readability and follows common conventions.
Suggested change
const {agentProvider, fixtureManager} = require('../../utils/e2e-framework'); const assert = require('assert/strict'); const configUtils = require('../../utils/config-utils'); +const jwt = require('jsonwebtoken');Then at line 48-49:
- const jwt = require('jsonwebtoken'); - const decoded = jwt.decode(response.body.featurebase.token); + const decoded = jwt.decode(response.body.featurebase.token);apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx (1)
22-30: Minor redundancy in billing check.
isProSite(line 22) already checksconfig?.config.hostSettings?.billing?.enabled, but line 29 checks the same condition again forshowGhostPro. Consider simplifying:- const showGhostPro = isOwnerUser(currentUser) && config?.config.hostSettings?.billing?.enabled; + const showGhostPro = isOwnerUser(currentUser) && isProSite;This doesn't affect correctness but improves clarity by reusing the already-computed value.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
apps/admin-x-framework/src/api/config.tsapps/admin-x-framework/src/api/featurebase.tsapps/admin-x-framework/src/index.tsapps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxapps/admin/src/hooks/use-featurebase.tsapps/admin/src/layout/app-sidebar/nav-ghost-pro.tsxapps/admin/src/layout/app-sidebar/nav-menu-item.tsxghost/core/core/server/api/endpoints/featurebase.jsghost/core/core/server/api/endpoints/index.jsghost/core/core/server/api/endpoints/utils/serializers/output/config.jsghost/core/core/server/api/endpoints/utils/serializers/output/featurebase.jsghost/core/core/server/api/endpoints/utils/serializers/output/index.jsghost/core/core/server/services/public-config/config.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/shared/labs.jsghost/core/test/e2e-api/admin/featurebase.test.jsghost/core/test/unit/server/services/public-config/config.test.js
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 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.
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: API routes should be defined in `ghost/core/core/server/api/`
Applied to files:
ghost/core/core/server/web/api/endpoints/admin/routes.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
ghost/core/core/server/web/api/endpoints/admin/routes.jsapps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2026-01-08T10:26:38.700Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25791
File: ghost/core/core/server/api/endpoints/member-comment-ban.js:64-68
Timestamp: 2026-01-08T10:26:38.700Z
Learning: In the Ghost API, endpoints rely on the serialization layer to prepare frame.data[docName] as a non-empty array before query() executes. Endpoints access frame.data[docName][0] directly (e.g., frame.data.comment_bans[0], frame.data.members[0], frame.data.posts[0]) without per-endpoint validation. This pattern is common across API endpoints. When maintaining or creating endpoints, avoid duplicating validation for frame.data[docName] and ensure the serializer guarantees the shape and non-emptiness. If you add a new endpoint that uses this frame.data[docName], follow the same assumption and avoid redundant checks unless there's a documented exception.
Applied to files:
ghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/core/server/api/endpoints/utils/serializers/output/featurebase.jsghost/core/core/server/api/endpoints/utils/serializers/output/config.jsghost/core/core/server/services/public-config/config.jsghost/core/core/server/api/endpoints/index.jsghost/core/test/unit/server/services/public-config/config.test.jsghost/core/core/server/api/endpoints/featurebase.jsghost/core/core/server/api/endpoints/utils/serializers/output/index.jsghost/core/test/e2e-api/admin/featurebase.test.jsghost/core/core/shared/labs.js
📚 Learning: 2025-06-10T11:07:10.800Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 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.
Applied to files:
ghost/core/core/server/services/public-config/config.jsapps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxapps/admin/src/layout/app-sidebar/nav-ghost-pro.tsxghost/core/core/shared/labs.js
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/hooks/**/*.{ts,tsx} : Custom React hooks should be placed in `src/hooks/*`
Applied to files:
apps/admin/src/hooks/use-featurebase.ts
📚 Learning: 2025-10-09T15:31:06.587Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
Applied to files:
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxghost/core/core/shared/labs.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
Applied to files:
ghost/core/test/unit/server/services/public-config/config.test.jsghost/core/test/e2e-api/admin/featurebase.test.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Analytics using Tinybird should reference scripts in `ghost/core/core/server/data/tinybird/scripts/` and datafiles in `ghost/core/core/server/data/tinybird/`
Applied to files:
ghost/core/core/server/api/endpoints/utils/serializers/output/index.js
📚 Learning: 2025-11-06T05:35:41.162Z
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-menu-item.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/index.ts : Place new UI components under `src/components/ui` and export them from `src/index.ts`
Applied to files:
apps/admin/src/layout/app-sidebar/nav-menu-item.tsxapps/admin-x-framework/src/index.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/e2e-api/admin/featurebase.test.js
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Manual login should not be used - authentication is automatic via fixture
Applied to files:
ghost/core/test/e2e-api/admin/featurebase.test.js
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2025-08-01T12:44:07.467Z
Learnt from: niranjan-uma-shankar
Repo: TryGhost/Ghost PR: 24557
File: apps/admin-x-settings/src/components/settings/general/TimeZone.tsx:7-7
Timestamp: 2025-08-01T12:44:07.467Z
Learning: In Ghost development, PRs may depend on unpublished changes from SDK packages. When this occurs, TypeScript compilation errors for missing exports are expected and documented in the PR description until the dependency packages are published and updated. This is normal workflow for cross-repository feature development.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
🧬 Code graph analysis (5)
ghost/core/core/server/services/public-config/config.js (3)
ghost/core/core/server/api/endpoints/featurebase.js (1)
config(3-3)ghost/core/core/server/services/email-analytics/jobs/index.js (1)
config(3-3)ghost/admin/tests/helpers/labs-flag.js (2)
config(10-10)config(29-29)
apps/admin/src/hooks/use-featurebase.ts (4)
apps/admin-x-framework/src/api/current-user.ts (1)
useCurrentUser(14-31)apps/admin-x-framework/src/api/config.ts (1)
useBrowseConfig(104-107)apps/admin/src/hooks/user-preferences.ts (1)
useUserPreferences(39-66)apps/admin-x-framework/src/api/featurebase.ts (1)
getFeaturebaseToken(14-17)
ghost/core/test/unit/server/services/public-config/config.test.js (1)
ghost/core/core/server/services/public-config/config.js (1)
configProperties(9-26)
apps/admin/src/layout/app-sidebar/nav-menu-item.tsx (2)
apps/shade/src/components/ui/sidebar.tsx (2)
SidebarMenuButton(762-762)useSidebar(772-772)apps/comments-ui/src/utils/helpers.ts (1)
isMobile(140-142)
ghost/core/test/e2e-api/admin/featurebase.test.js (2)
ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js (1)
fixtureManager(9-9)ghost/core/core/server/api/endpoints/featurebase.js (1)
jwt(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (23)
ghost/core/core/shared/labs.js (1)
59-60: LGTM!The new
featurebaseFeedbackflag is correctly added toPRIVATE_FEATURES, following the established pattern for private lab features. This properly gates the Featurebase integration behind developer experiments.apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx (1)
70-74: LGTM!The feature entry follows the established pattern and correctly references the
featurebaseFeedbackflag. The description appropriately notes the dependency on the new admin experience.ghost/core/core/server/services/public-config/config.js (1)
41-47: LGTM!The implementation correctly gates featurebase config exposure behind both the labs flag and config existence. The explicit filtering to only expose
enabledandorganization(public properties) is a good security practice, ensuring sensitive configuration like JWT secrets are not leaked to the client.apps/admin-x-framework/src/api/config.ts (1)
88-91: LGTM!The
featurebasetype definition correctly mirrors the backend's conditional config exposure with appropriate optional properties.ghost/core/core/server/api/endpoints/index.js (1)
192-194: LGTM!The featurebase endpoint getter follows the established pattern used by other endpoints in this file (e.g.,
tinybird).ghost/core/core/server/api/endpoints/utils/serializers/output/index.js (1)
146-150: LGTM!The new
featurebaseserializer getter follows the established lazy-loading pattern used by other serializers in this file (e.g.,tinybird).ghost/core/test/unit/server/services/public-config/config.test.js (1)
157-177: Well-designed security test.This test correctly validates that sensitive fields (
jwtSecret,apiKey) are not exposed in the public config while ensuring the public fields (enabled,organization) are available. The combination ofdeepEqualfor exact shape verification and explicitundefinedchecks provides good documentation of the expected behavior.apps/admin-x-framework/src/index.ts (1)
52-53: LGTM!The new Featurebase exports follow the established pattern used for Tinybird (lines 50-51), maintaining consistency in the framework's API surface.
ghost/core/core/server/api/endpoints/utils/serializers/output/config.js (1)
26-27: LGTM!Adding
featurebaseto the allowlist follows the established pattern. The serializer correctly relies on the upstreampublic-configservice to filter sensitive fields before serialization, as verified by the unit tests.ghost/core/core/server/api/endpoints/utils/serializers/output/featurebase.js (1)
1-11: LGTM!The serializer follows the established pattern used by the
tinybirdserializer, providing consistent API response structure. The debug namespace is appropriately scoped.ghost/core/core/server/web/api/endpoints/admin/routes.js (2)
25-25: LGTM!The new
/config/featurebaseroute follows the established pattern for config sub-routes and correctly appliesmw.authAdminApimiddleware.
260-262: LGTM!The
/featurebase/tokenroute is correctly modeled after the existing/tinybird/tokenroute pattern, with proper authentication middleware applied.apps/admin/src/layout/app-sidebar/nav-menu-item.tsx (2)
72-97: LGTM!The
NavMenuButtoncomponent correctly mirrors the mobile sidebar handling pattern fromNavMenuLinkand properly forwards props toSidebarMenuButton. The implementation is clean and consistent with the existing component structure.
101-103: LGTM!The compound component pattern assignment and export are consistent with the existing
NavMenuItem.LinkandNavMenuItem.Labelapproach.ghost/core/test/e2e-api/admin/featurebase.test.js (3)
1-11: LGTM!Test setup follows the established e2e test patterns with proper use of
agentProvider,fixtureManager, andconfigUtils.
20-102: Comprehensive configuration scenario coverage.The Owner tests thoroughly cover the key scenarios: valid config, disabled, unconfigured, and missing secret. The JWT payload structure verification (name, email, companies) aligns with expected Featurebase identification requirements.
104-180: LGTM!Role-based access control tests correctly verify that Admin, Editor, and Author can obtain tokens while Contributors are forbidden. This aligns with the PR objective that all staff except Contributors can submit feedback.
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx (1)
40-55: LGTM!The conditional rendering correctly displays the Ghost(Pro) link for owners with billing enabled, and the Feedback button for non-contributor staff when Featurebase is enabled. The use of
NavMenuItem.ButtonwithopenFeedbackWidgetintegrates cleanly with the new hook.apps/admin-x-framework/src/api/featurebase.ts (1)
1-17: LGTM!The API helper follows the established pattern for typed queries in the framework. The interface definitions are clean, and the
createQueryconfiguration correctly targets the/featurebase/token/endpoint. This aligns well with the backend route implementation.ghost/core/core/server/api/endpoints/featurebase.js (2)
35-41: Permission check implementation looks correct.The custom permissions function properly loads user roles and denies access to Contributors, aligning with the PR objective that "all staff except Contributors can submit feedback."
42-57: Configuration validation and token generation flow is sound.The query properly validates that Featurebase is both enabled and has a JWT secret configured before generating the token. The error handling with
ValidationErroris appropriate for misconfiguration scenarios.apps/admin/src/hooks/use-featurebase.ts (2)
56-77: Initialization pattern prevents retry and theme updates.Two observations:
Setting
isInitializedRef.current = truebefore the async SDK load completes means if loading fails, the widget won't retry on subsequent renders.The
preferences?.nightShiftis in the dependency array, but due to theisInitializedRefguard, the theme won't update if the user toggles dark mode after initialization.Both are likely acceptable trade-offs for simplicity, but worth confirming they align with expected behavior.
79-87: Verify postMessage target origin security.Using
'*'as the target origin forpostMessageis common for same-window SDK communication, but it allows any origin to receive the message if the page is embedded. Since this is an admin interface unlikely to be embedded, the risk is low—just confirming this is intentional.
| return jwt.sign(userData, config.get('featurebase:jwtSecret'), { | ||
| algorithm: 'HS256' | ||
| }); |
There was a problem hiding this comment.
JWT token has no expiration.
The token is signed without an expiresIn option, meaning it remains valid indefinitely. If a token is compromised, it cannot be automatically invalidated through expiration.
Consider adding a reasonable expiration time:
🔒 Proposed fix
- return jwt.sign(userData, config.get('featurebase:jwtSecret'), {
- algorithm: 'HS256'
- });
+ return jwt.sign(userData, config.get('featurebase:jwtSecret'), {
+ algorithm: 'HS256',
+ expiresIn: '24h'
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return jwt.sign(userData, config.get('featurebase:jwtSecret'), { | |
| algorithm: 'HS256' | |
| }); | |
| return jwt.sign(userData, config.get('featurebase:jwtSecret'), { | |
| algorithm: 'HS256', | |
| expiresIn: '24h' | |
| }); |
🤖 Prompt for AI Agents
In @ghost/core/core/server/api/endpoints/featurebase.js around lines 20 - 22,
The JWT created in the jwt.sign call inside featurebase.js is missing an
expiration; update the jwt.sign invocation to include an expiresIn option (e.g.
{ algorithm: 'HS256', expiresIn: <value> }) and source the expiry from
configuration (add/read e.g. config.get('featurebase:jwtExpiry') with a sensible
default like '1h') so tokens automatically expire; ensure any config schema or
consumers are updated to accept the new config key.
There was a problem hiding this comment.
Featurebase's docs don't mention expiration at all. We only need a token to be valid for the lifetime of an Admin session (i.e. time the tab is open in a browser) but that varies wildly and could be on the order of a week or more for pinned tabs. Something we'd like to avoid is the feedback button not working in a long-lived session because the token requested at the start of the session has expired
There was a problem hiding this comment.
@kevinansfield, thank you for the clarification! That makes perfect sense for this use case. Having the token remain valid for long-lived admin sessions ensures a smooth user experience, especially for users with pinned tabs who may go days without a page refresh.
Since Featurebase itself doesn't require or document token expiration, and the token is tied to the admin session lifecycle, leaving it without an expiry is the right approach here.
✏️ Learnings added
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25801
File: ghost/core/core/server/api/endpoints/featurebase.js:20-22
Timestamp: 2026-01-13T16:44:31.506Z
Learning: In the Featurebase JWT token endpoint (`ghost/core/core/server/api/endpoints/featurebase.js`), JWT tokens are intentionally created without an expiration time. This is because tokens need to remain valid for the entire lifetime of an admin session, which can last a week or more for pinned browser tabs. Featurebase's documentation does not mention token expiration, and adding expiration would break the feedback widget in long-lived sessions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: 9larsons
Repo: TryGhost/Ghost PR: 21866
File: ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js:10-19
Timestamp: 2025-04-23T15:44:52.549Z
Learning: The existing implementation in `ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js` using `path.parse(req.url).base` is secure against path traversal attacks as it properly extracts only the filename component without any directory parts.
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25075
File: ghost/admin/app/services/whats-new.js:77-83
Timestamp: 2025-10-09T08:10:18.956Z
Learning: In ghost/admin/app/services/whats-new.js, reusing `this._whatsNewSettings` (initialized once in `init()`) when persisting defaults for new users without `lastSeenDate` is acceptable. The team is aware that this means sequential logins by different admins in the same tab could share the service initialization timestamp, but this tradeoff is not a concern for their use case.
There was a problem hiding this comment.
If a long-lived token is problematic then we could change the approach to have short-lived tokens and delay featurebase initialization until the widget is triggered. However, we don't want to introduce any lengthy delays between clicking the menu item and the widget opening if network conditions aren't ideal
There was a problem hiding this comment.
@kevinansfield, the long-lived token approach you've implemented is appropriate for this use case. While long-lived tokens do carry some inherent risk if compromised, the tradeoffs here are well-justified:
Benefits of current approach:
- No delays when opening the feedback widget (better UX)
- Simpler implementation
- Works seamlessly with long-lived admin sessions and pinned tabs
- Aligns with how Featurebase expects to be used
Risk mitigation factors:
- Tokens are only issued to authenticated admin users (non-Contributors)
- The token is used client-side for a specific, limited purpose (feedback widget)
- Admin sessions already require authentication
- The alternative (delayed initialization with short-lived tokens) would add complexity and could introduce noticeable latency
Given that Featurebase doesn't document or require token expiration, and the primary goal is a smooth user experience in long-lived sessions, your current implementation strikes the right balance. The alternative you mentioned would trade security benefit for UX friction without significant real-world security gain in this context.
🧠 Learnings used
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25801
File: ghost/core/core/server/api/endpoints/featurebase.js:20-22
Timestamp: 2026-01-13T16:44:31.506Z
Learning: In the Featurebase JWT token endpoint (`ghost/core/core/server/api/endpoints/featurebase.js`), JWT tokens are intentionally created without an expiration time. This is because tokens need to remain valid for the entire lifetime of an admin session, which can last a week or more for pinned browser tabs. Featurebase's documentation does not mention token expiration, and adding expiration would break the feedback widget in long-lived sessions.
Learnt from: 9larsons
Repo: TryGhost/Ghost PR: 21866
File: ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js:10-19
Timestamp: 2025-04-23T15:44:52.549Z
Learning: The existing implementation in `ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js` using `path.parse(req.url).base` is secure against path traversal attacks as it properly extracts only the filename component without any directory parts.
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25075
File: ghost/admin/app/services/whats-new.js:77-83
Timestamp: 2025-10-09T08:10:18.956Z
Learning: In ghost/admin/app/services/whats-new.js, reusing `this._whatsNewSettings` (initialized once in `init()`) when persisting defaults for new users without `lastSeenDate` is acceptable. The team is aware that this means sequential logins by different admins in the same tab could share the service initialization timestamp, but this tradeoff is not a concern for their use case.
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25358
File: apps/admin/src/whats-new/hooks/use-whats-new.ts:0-0
Timestamp: 2025-11-10T20:44:33.377Z
Learning: In the Ghost admin app's What's New feature, the changelog query (useChangelog) is configured with `staleTime: infinity` and is intended to be fetched only once per app load, so it never invalidates during a session.
There was a problem hiding this comment.
Settled on a 7-day expiration and automatic re-fetch in the client.
Featurebase themselves list expirations as "coming soon" but testing shows tokens with expiration dates are accepted. With the 7-day expiration and auto-refetch the likelihood of making requests to Featurebase with an expired token is extremely low.
There was a problem hiding this comment.
@kevinansfield, that's a great solution! The 7-day expiration with automatic client-side re-fetch strikes an excellent balance:
✅ Security: Tokens now have a defined expiration, limiting the risk window if compromised
✅ UX: Long enough to support pinned tabs and extended sessions without interruption
✅ Technical: Avoids React Query auto-refetch issues and JavaScript's setTimeout max value limitations
✅ Compatibility: Testing confirms Featurebase accepts tokens with expiration dates
The combination of a week-long validity period and proactive re-fetching makes expired token scenarios extremely unlikely while maintaining seamless user experience. This addresses the security concern I originally flagged while respecting the real-world usage patterns of long-lived admin sessions.
✏️ Learnings added
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25801
File: ghost/core/core/server/api/endpoints/featurebase.js:20-23
Timestamp: 2026-01-14T11:27:20.799Z
Learning: In the Featurebase JWT token endpoint (`ghost/core/core/server/api/endpoints/featurebase.js`), JWT tokens are created with a 7-day expiration time using `expiresIn: '7d'`. The client automatically re-fetches tokens to ensure they remain valid throughout long-lived admin sessions (which can last a week or more for pinned browser tabs). This approach balances security with UX and avoids React Query auto-refetch and JavaScript `setTimeout` limitations that would occur with longer or infinite token lifetimes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25801
File: ghost/core/core/server/api/endpoints/featurebase.js:20-22
Timestamp: 2026-01-13T16:44:40.934Z
Learning: In the Featurebase JWT token endpoint (`ghost/core/core/server/api/endpoints/featurebase.js`), JWT tokens are intentionally created without an expiration time. This is because tokens need to remain valid for the entire lifetime of an admin session, which can last a week or more for pinned browser tabs. Featurebase's documentation does not mention token expiration, and adding expiration would break the feedback widget in long-lived sessions.
🧠 Learnings used
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25801
File: ghost/core/core/server/api/endpoints/featurebase.js:20-22
Timestamp: 2026-01-13T16:44:40.934Z
Learning: In the Featurebase JWT token endpoint (`ghost/core/core/server/api/endpoints/featurebase.js`), JWT tokens are intentionally created without an expiration time. This is because tokens need to remain valid for the entire lifetime of an admin session, which can last a week or more for pinned browser tabs. Featurebase's documentation does not mention token expiration, and adding expiration would break the feedback widget in long-lived sessions.
Learnt from: 9larsons
Repo: TryGhost/Ghost PR: 21866
File: ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js:10-19
Timestamp: 2025-04-23T15:44:52.549Z
Learning: The existing implementation in `ghost/core/core/server/web/admin/middleware/serve-auth-frame-file.js` using `path.parse(req.url).base` is secure against path traversal attacks as it properly extracts only the filename component without any directory parts.
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25075
File: ghost/admin/app/services/whats-new.js:77-83
Timestamp: 2025-10-09T08:10:18.956Z
Learning: In ghost/admin/app/services/whats-new.js, reusing `this._whatsNewSettings` (initialized once in `init()`) when persisting defaults for new users without `lastSeenDate` is acceptable. The team is aware that this means sequential logins by different admins in the same tab could share the service initialization timestamp, but this tradeoff is not a concern for their use case.
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25358
File: apps/admin/src/whats-new/hooks/use-whats-new.ts:0-0
Timestamp: 2025-11-10T20:44:33.377Z
Learning: In the Ghost admin app's What's New feature, the changelog query (useChangelog) is configured with `staleTime: infinity` and is intended to be fetched only once per app load, so it never invalidates during a session.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 3
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/admin/src/hooks/use-featurebase.ts:
- Around line 63-90: The initialization flag is set too early: move setting
isInitializedRef.current and updating lastThemeRef.current to occur only after
successful SDK load and widget initialization (i.e., inside the
loadFeaturebaseSDK().then(...) success callback after
window.Featurebase('initialize_feedback_widget') completes), and in the .catch
or initialization error callback reset isInitializedRef.current = false (and
optionally revert lastThemeRef.current) so failed loads don't block future
retries; reference isInitializedRef, lastThemeRef, loadFeaturebaseSDK, and
window.Featurebase when making this change.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/admin/src/hooks/use-featurebase.tsapps/admin/src/layout/app-sidebar/nav-ghost-pro.tsxghost/core/core/server/api/endpoints/featurebase.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/api/endpoints/featurebase.js
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-10T11:07:10.800Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 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.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2025-08-01T12:44:07.467Z
Learnt from: niranjan-uma-shankar
Repo: TryGhost/Ghost PR: 24557
File: apps/admin-x-settings/src/components/settings/general/TimeZone.tsx:7-7
Timestamp: 2025-08-01T12:44:07.467Z
Learning: In Ghost development, PRs may depend on unpublished changes from SDK packages. When this occurs, TypeScript compilation errors for missing exports are expected and documented in the PR description until the dependency packages are published and updated. This is normal workflow for cross-repository feature development.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Ghost-CLI tests
- GitHub Check: ActivityPub tests
- GitHub Check: Lint
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (4)
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx (2)
21-27: LGTM on the feature gating logic.The early return for missing
currentUserprevents potential null reference issues. The computed conditions cleanly separate concerns:showGhostProgates the billing feature to owners on Pro sites, whileshowFeedbackproperly combines the feature flag, role check (excluding contributors), and config enablement.
37-52: LGTM on the conditional rendering.The conditional rendering is clean and the two features are properly isolated. The
NavMenuItem.Buttoncomponent for the Feedback item appropriately uses a button element (versus a link) since it triggers widget behavior rather than navigation.apps/admin/src/hooks/use-featurebase.ts (2)
19-39: LGTM on SDK loading with queue mechanism.The queue pattern at lines 33-37 correctly handles race conditions where the SDK might be called before the script finishes loading. The script existence check combined with the queue ensures calls are buffered and processed once the SDK is ready.
92-100: Featurebase SDK postMessage API is correctly implemented.The
openFeedbackWidgetfunction properly uses the documented Featurebase SDK interface with correct target'FeaturebaseWidget', action'openFeedbackWidget', and optionalsetBoardparameter.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/admin/src/hooks/use-featurebase.ts (1)
87-94: Consider cleaner error handling structure.The current pattern sets
initializedThemeRef.current = themethen immediately resets it tonullon error. While functional, an if/else would be clearer.♻️ Suggested improvement
}, (err) => { isInitializingRef.current = false; - initializedThemeRef.current = theme; - if (err) { console.error('[Featurebase] Failed to initialize widget:', err); initializedThemeRef.current = null; + } else { + initializedThemeRef.current = theme; } });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/admin/src/hooks/use-featurebase.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-09T15:31:06.587Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
Applied to files:
apps/admin/src/hooks/use-featurebase.ts
📚 Learning: 2025-07-14T12:20:33.264Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24346
File: apps/admin-x-settings/src/components/settings/growth/Network.tsx:16-16
Timestamp: 2025-07-14T12:20:33.264Z
Learning: In the Network component at apps/admin-x-settings/src/components/settings/growth/Network.tsx, the hardcoded `domainIssue = true` is intentional for static UI implementation, as confirmed by the maintainer sagzy. This is acceptable for early UI development before connecting to actual backend validation.
Applied to files:
apps/admin/src/hooks/use-featurebase.ts
🧬 Code graph analysis (1)
apps/admin/src/hooks/use-featurebase.ts (4)
apps/admin-x-framework/src/api/current-user.ts (1)
useCurrentUser(14-31)apps/admin-x-framework/src/api/config.ts (1)
useBrowseConfig(104-107)apps/admin/src/hooks/user-preferences.ts (1)
useUserPreferences(39-66)apps/admin-x-framework/src/api/featurebase.ts (1)
getFeaturebaseToken(14-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Lint
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: ActivityPub tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (4)
apps/admin/src/hooks/use-featurebase.ts (4)
8-15: LGTM!Type declarations correctly model the Featurebase SDK API, including the queue mechanism for calls made before the SDK fully loads.
19-45: LGTM!Good implementation with proper retry handling - removing the script on error (line 31) allows subsequent attempts to retry the load. The queue setup ensures calls made during SDK loading are properly queued.
51-67: LGTM!Clean setup with proper conditional data fetching - the token query is only enabled when the feature is active, and refs appropriately track initialization state to prevent duplicate initialization.
102-110: LGTM - standard widget communication pattern.Using
postMessagewith'*'as the target origin is common for same-page widget communication. Since no sensitive data is transmitted (just UI actions), the security impact is minimal. The empty dependency array correctly produces a stable callback reference.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts (2)
19-45: Potential race condition when script is still loading.If
loadFeaturebaseSDK()is called while the script element exists but hasn't finished loading yet, the function resolves immediately (line 23) without waiting for the actual load to complete. A second initialization attempt could then try to usewindow.Featurebasebefore the SDK is ready.Consider tracking the load state with a module-level promise:
♻️ Suggested approach
const SDK_URL = 'https://do.featurebase.app/js/sdk.js'; +let sdkLoadPromise: Promise<void> | null = null; function loadFeaturebaseSDK(): Promise<void> { + if (sdkLoadPromise) { + return sdkLoadPromise; + } + + sdkLoadPromise = new Promise((resolve, reject) => { - return new Promise((resolve, reject) => { const existingScript = document.querySelector(`script[src="${SDK_URL}"]`); if (existingScript) { resolve(); return; } // ... rest of implementation - }); + }).catch((err) => { + sdkLoadPromise = null; + throw err; + }); + + return sdkLoadPromise; }That said, the existing
isInitializingRefguard in theuseEffectlikely prevents this from being an issue in practice.
102-110: Consider documenting the'*'targetOrigin usage.Using
'*'withpostMessageis typically discouraged, but appears acceptable here since:
- The message targets a specific widget (
'FeaturebaseWidget')- The payload contains no sensitive data
- This is likely the expected Featurebase SDK communication pattern
A brief comment explaining this is the Featurebase-prescribed API would aid future maintainers.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.tsapps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25801
File: ghost/core/core/server/api/endpoints/featurebase.js:20-22
Timestamp: 2026-01-13T16:44:31.506Z
Learning: In the Featurebase JWT token endpoint (`ghost/core/core/server/api/endpoints/featurebase.js`), JWT tokens are intentionally created without an expiration time. This is because tokens need to remain valid for the entire lifetime of an admin session, which can last a week or more for pinned browser tabs. Featurebase's documentation does not mention token expiration, and adding expiration would break the feedback widget in long-lived sessions.
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
📚 Learning: 2025-06-10T11:07:10.800Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 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.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2025-08-01T12:44:07.467Z
Learnt from: niranjan-uma-shankar
Repo: TryGhost/Ghost PR: 24557
File: apps/admin-x-settings/src/components/settings/general/TimeZone.tsx:7-7
Timestamp: 2025-08-01T12:44:07.467Z
Learning: In Ghost development, PRs may depend on unpublished changes from SDK packages. When this occurs, TypeScript compilation errors for missing exports are expected and documented in the PR description until the dependency packages are published and updated. This is normal workflow for cross-repository feature development.
Applied to files:
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx
📚 Learning: 2025-10-09T15:31:06.587Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
Applied to files:
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts
📚 Learning: 2025-07-14T12:20:33.264Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24346
File: apps/admin-x-settings/src/components/settings/growth/Network.tsx:16-16
Timestamp: 2025-07-14T12:20:33.264Z
Learning: In the Network component at apps/admin-x-settings/src/components/settings/growth/Network.tsx, the hardcoded `domainIssue = true` is intentional for static UI implementation, as confirmed by the maintainer sagzy. This is acceptable for early UI development before connecting to actual backend validation.
Applied to files:
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts
🧬 Code graph analysis (2)
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx (5)
apps/shade/src/components/ui/sidebar.tsx (3)
SidebarGroup(752-752)SidebarGroupContent(754-754)SidebarMenu(759-759)apps/admin-x-framework/src/api/config.ts (1)
useBrowseConfig(104-107)apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts (1)
useFeaturebase(51-113)apps/admin-x-framework/src/api/users.ts (2)
isOwnerUser(159-161)isContributorUser(181-183)apps/admin/src/layout/app-sidebar/nav-menu-item.tsx (1)
NavMenuItem(103-103)
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts (3)
apps/admin-x-framework/src/api/current-user.ts (1)
useCurrentUser(14-31)apps/admin-x-framework/src/api/config.ts (1)
useBrowseConfig(104-107)apps/admin/src/hooks/user-preferences.ts (1)
useUserPreferences(39-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Ghost-CLI tests
- GitHub Check: ActivityPub tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: i18n
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Build & Push Docker Image
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
apps/admin/src/layout/app-sidebar/nav-ghost-pro.tsx (3)
1-13: LGTM!Imports are well-organized. The new imports for
isContributorUser,useFeatureFlag, anduseFeaturebasealign with the feature requirements.
15-31: LGTM!Good defensive pattern - hooks are called unconditionally before early returns, satisfying React's rules of hooks. The permission logic correctly gates:
- Ghost(Pro) to owners with billing enabled
- Feedback to non-contributor staff when Featurebase is configured and the feature flag is active
33-56: LGTM!Clean conditional rendering. The new
NavMenuItem.Buttoncomponent appropriately handles the click action for the feedback widget, maintaining consistency with the existingNavMenuItem.Linkpattern for the Ghost(Pro) item.apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts (3)
1-15: LGTM!Type declarations are well-structured. The global
Windowaugmentation is appropriately scoped and necessary for the third-party SDK integration.
51-100: LGTM!The initialization logic is well-guarded:
- Multiple prerequisite checks prevent premature initialization
isInitializingRefprevents concurrent initialization attemptsinitializedThemeRefenables theme-change re-initialization while avoiding redundant calls- Error handling properly resets state on failure
Based on learnings, the JWT token intentionally lacks an expiration to support long-lived admin sessions.
112-113: LGTM!Clean, minimal API surface that exposes only the necessary functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts (1)
19-45: Potential race condition when script tag exists but SDK hasn't loaded.If multiple components call
loadFeaturebaseSDK()simultaneously, or if a previous load attempt is still in progress, this function resolves immediately upon finding an existing script tag without waiting for the SDK to actually be ready. The optional chaining at line 84 (window.Featurebase?.()) prevents crashes but causes silent initialization failures.Consider tracking load state or waiting for the existing script's load event:
♻️ Suggested improvement
function loadFeaturebaseSDK(): Promise<void> { return new Promise((resolve, reject) => { + // SDK already loaded and ready + if (typeof window.Featurebase === 'function' && !window.Featurebase.q) { + resolve(); + return; + } + const existingScript = document.querySelector(`script[src="${SDK_URL}"]`); if (existingScript) { - resolve(); - return; + // Script tag exists but SDK may not be loaded yet - wait for it + if (typeof window.Featurebase === 'function' && !window.Featurebase.q) { + resolve(); + } else { + existingScript.addEventListener('load', () => resolve()); + existingScript.addEventListener('error', () => reject(new Error('[Featurebase] SDK failed to load'))); + } + return; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/features/**/*.{ts,tsx} : Higher-level, opinionated components (e.g., PostShareModal, SourceTabs) should be placed in `src/components/features/*`
Applied to files:
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/hooks/**/*.{ts,tsx} : Custom React hooks should be placed in `src/hooks/*`
Applied to files:
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts
📚 Learning: 2025-10-09T15:31:06.587Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25118
File: apps/portal/src/actions.js:160-173
Timestamp: 2025-10-09T15:31:06.587Z
Learning: When reviewing PRs that introduce feature-flagged changes (e.g., `labs?.membersSigninOTCAlpha`), avoid suggesting modifications to non-flagged code paths unless they're directly related to the PR's objectives. Keep the scope focused on the feature-flag-specific changes only.
Applied to files:
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts
📚 Learning: 2025-10-27T11:59:33.968Z
Learnt from: peterzimon
Repo: TryGhost/Ghost PR: 25261
File: apps/admin/src/layout/app-sidebar/UserMenu.tsx:24-29
Timestamp: 2025-10-27T11:59:33.968Z
Learning: In apps/admin/src/layout/app-sidebar/UserMenu.tsx, the hardcoded placeholder data (avatar URL, initials "US", name "User Name", email "userexample.com") is a known limitation and should not be flagged for replacement with real user data.
Applied to files:
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts
📚 Learning: 2026-01-13T16:44:31.506Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 25801
File: ghost/core/core/server/api/endpoints/featurebase.js:20-22
Timestamp: 2026-01-13T16:44:31.506Z
Learning: In the Featurebase JWT token endpoint (`ghost/core/core/server/api/endpoints/featurebase.js`), JWT tokens are intentionally created without an expiration time. This is because tokens need to remain valid for the entire lifetime of an admin session, which can last a week or more for pinned browser tabs. Featurebase's documentation does not mention token expiration, and adding expiration would break the feedback widget in long-lived sessions.
Applied to files:
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts
🧬 Code graph analysis (1)
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts (3)
apps/admin-x-framework/src/api/current-user.ts (1)
useCurrentUser(14-31)apps/admin-x-framework/src/api/config.ts (1)
useBrowseConfig(104-107)apps/admin/src/hooks/user-preferences.ts (1)
useUserPreferences(39-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Lint
- GitHub Check: Ghost-CLI tests
- GitHub Check: ActivityPub tests
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (3)
apps/admin/src/layout/app-sidebar/hooks/use-featurebase.ts (3)
69-103: Initialization logic is well-structured.Good use of refs to track initialization state and prevent redundant SDK reinitializations. The re-initialization on theme/token change correctly handles user preference updates and auth changes as mentioned in the PR objectives.
105-113: LGTM on widget communication.Using
postMessagewith'*'as targetOrigin is the expected pattern for communicating with embedded widget iframes where the exact origin may vary. The data transmitted (action and board name) is non-sensitive.
64-67: No action needed. Theget*naming forgetFeaturebaseTokenis an intentional framework design pattern used consistently across admin-x-framework for simpler query hooks, whileuse*is reserved for mutations and complex queries. Both follow proper React hook semantics via thecreateQuery()factory.Likely an incorrect or invalid review comment.
- we don't want to add custom `permissions` functions, fixtures would be preferable in general but it's added complexity for this use case - we're happy with gating the Feedback widget access on the client rather backend+client
- with auto-refetch on the query, `token` changes and triggers re-initialisation of SDK via the `useEffect` hook
- still likely more than enough and avoids issues with max `setTimeout` of 24.8 days for React Query's auto refetch
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
I had a quick look and I have no big objections. Maybe we can do a deeper post-merge review if you want to get this in today?
| type NavMenuButtonProps = React.ComponentProps<typeof SidebarMenuButton> & { | ||
| onClick?: () => void | ||
| }; | ||
| function NavMenuButton({ | ||
| onClick, | ||
| children, | ||
| ...props | ||
| }: NavMenuButtonProps) { | ||
| const { isMobile, setOpenMobile } = useSidebar(); | ||
|
|
||
| const handleClick = () => { | ||
| if (isMobile) { | ||
| setOpenMobile(false); | ||
| } | ||
| onClick?.(); | ||
| }; | ||
|
|
||
| return ( | ||
| <SidebarMenuButton | ||
| onClick={handleClick} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </SidebarMenuButton> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this actually used anywhere?
There was a problem hiding this comment.
2 participants
ref https://linear.app/ghost/issue/FEA-509/
Adds a "Feedback" menu item to the admin sidebar when configured, allowing staff users to submit bug reports and feedback without leaving the admin interface.
featurebaseFeedbacklabs flag/api/admin/featurebase/token/endpoint that returns an identification JWT for secure use by featurebase widgetsgetFeaturebaseTokenAPI query hook in admin-x-frameworktokendata will re-run without an outside triggered re-render, enablesuseFeaturebaseto re-initialize the SDK when the token expiresfeaturebaseproperty to the/api/admin/config/endpoint, exposing only the public data needed for the integration to functionuseFeaturebasehook to Admin app which manages Featurebase instantiation and identificationopenFeedbackWidget()method ready for use via buttons etcNavMenuItem.Buttoncomponent to support button menu items in addition to link menu itemsFeedbackbutton to sidebar menu when configured+enabled and staff user has sufficient permissions