Uh oh!
There was an error while loading. Please reload this page.
fix(web): hide upgrade prompts from non-owner users in the sidebar - #1525
Closed
Harsh23Kashyap wants to merge 2 commits into
Closed
fix(web): hide upgrade prompts from non-owner users in the sidebar#1525Harsh23Kashyap wants to merge 2 commits into
Harsh23Kashyap wants to merge 2 commits into
Conversation
The "Upgrade to Pro" button in the sidebar footer and the per-item "Upgrade" badges in the default and settings sidebars were rendered to any signed-in user, not just to the org's OWNER. A MEMBER (or a former owner who lost the role) would see prompts they cannot act on. The settings pages already gate on OWNER via authenticatedPage, so the upsell cards inside /settings/security and /settings/audit were unaffected — only the shared sidebar was missing the check. Fix: thread an isOwner prop from the sidebar index files through the Nav and SidebarBase components. defaultSidebar/index.tsx already computed isOwner for the connection-stats notification dot; settingsSidebar/index.tsx now does the same. Both UpgradeBadge rendering in the Nav components and UpgradeButton rendering in SidebarBase are gated on isOwner. Issue #1524.
Contributor
WalkthroughThis PR gates the sidebar footer "Upgrade to Pro" button and per-item "Upgrade" badges to organization owners only. It adds an ChangesOwner-gated upgrade UI
Estimated code review effort: 2 (Simple) | ~14 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/app/(app)/@sidebar/components/defaultSidebar/nav.test.tsx (1)
73-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for the settings nav and footer CTA.
This file renders only
defaultSidebar/Nav. It does not exercisesettingsSidebar/Nav,SidebarBase, or the owner-state wiring in either index component. A regression in those gates could pass this suite. Add owner, member, and unauthenticated cases for the settings navigation, plus owner and non-owner cases forSidebarBasewhenisValidLicenseActiveisfalse.🤖 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 `@packages/web/src/app/`(app)/@sidebar/components/defaultSidebar/nav.test.tsx around lines 73 - 98, The current tests cover only defaultSidebar/Nav; extend the suite to exercise settingsSidebar/Nav with owner, member, and unauthenticated cases, verifying the upgrade badge appears only for owners. Also add SidebarBase coverage with isValidLicenseActive=false for both owner and non-owner states, asserting the owner-state gate and footer CTA behavior.
🤖 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.
Nitpick comments:
In `@packages/web/src/app/`(app)/@sidebar/components/defaultSidebar/nav.test.tsx:
- Around line 73-98: The current tests cover only defaultSidebar/Nav; extend the
suite to exercise settingsSidebar/Nav with owner, member, and unauthenticated
cases, verifying the upgrade badge appears only for owners. Also add SidebarBase
coverage with isValidLicenseActive=false for both owner and non-owner states,
asserting the owner-state gate and footer CTA behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f74121a2-6550-44f0-9c40-38b9b0520532
📒 Files selected for processing (7)
CHANGELOG.mdpackages/web/src/app/(app)/@sidebar/components/defaultSidebar/index.tsxpackages/web/src/app/(app)/@sidebar/components/defaultSidebar/nav.test.tsxpackages/web/src/app/(app)/@sidebar/components/defaultSidebar/nav.tsxpackages/web/src/app/(app)/@sidebar/components/settingsSidebar/index.tsxpackages/web/src/app/(app)/@sidebar/components/settingsSidebar/nav.tsxpackages/web/src/app/(app)/@sidebar/components/sidebarBase.tsx
CodeRabbit nitpick: the original test only covered the defaultSidebar Nav. Extend the suite to cover the other two owner-gated surfaces that the fix touches: - settingsSidebar/nav.test.tsx (2 cases): badge renders for an OWNER missing the entitlement, hidden for a non-owner. Uses two mock nav groups (one gated, one ungated) so the test also verifies that the ungated item is not affected. - sidebarBase.test.tsx (3 cases): UpgradeButton renders for an OWNER with no valid license, hidden for a non-owner with no valid license, hidden for an OWNER with a valid license. The last case locks in the pre-existing !isValidLicenseActive gate so the isOwner refactor doesn't accidentally drop it. The SidebarBase test mocks the heavy client-only dependencies (theme, keymap, PostHog, signOut, dropdown menu primitives) so the test only exercises the upgrade CTA gate, which is the only behaviour this PR changes. 8/8 sidebar tests pass; full suite 1005/1005 (5 new tests added since the original 1000 baseline; the 7 pre-existing OTel-setup failures in ee/askmcp and ee/permissionSyncStatus are unchanged by this PR). Refs #1524.
Harsh23Kashyap
commented
Jul 31, 2026
Author
Extended the test coverage in 680bfb82:
8/8 sidebar tests pass; full suite 1005/1005 (5 new tests since the original 1000 baseline; the 7 pre-existing OTel-setup failures in ee/askmcp and ee/permissionSyncStatus are unchanged). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The "Upgrade to Pro" button in the sidebar footer and the per-item "Upgrade" badges in the default and settings sidebars were rendered to any signed-in user, not just to the org's OWNER. A MEMBER (or a former owner who lost the role) would see prompts they cannot act on.
Fixes#1524.
What changes
Thread an
isOwnerprop from the sidebar index files through theNavandSidebarBasecomponents. BothUpgradeBadgerendering in the twoNavcomponents andUpgradeButtonrendering inSidebarBaseare now gated onisOwner.defaultSidebar/index.tsxalready computedisOwnerfor the connection-stats notification dot — that value is now also passed to<Nav>and<SidebarBase>.settingsSidebar/index.tsxdid not compute it at all; it now does (using the samegetAuthContext+OrgRole.OWNERpattern) and threads it through.Files
packages/web/src/app/(app)/@sidebar/components/sidebarBase.tsx— addedisOwner?: booleantoSidebarBaseProps, gated theUpgradeButtononisOwner && !isValidLicenseActive.packages/web/src/app/(app)/@sidebar/components/defaultSidebar/nav.tsx— addedisOwner?: booleantoNavProps, gatedshowUpgradeBadgeonisOwner && missingEntitlement.packages/web/src/app/(app)/@sidebar/components/defaultSidebar/index.tsx— passisOwner={isOwner}to<Nav>and<SidebarBase>(the value was already computed at line 43).packages/web/src/app/(app)/@sidebar/components/settingsSidebar/nav.tsx— same pattern asdefaultSidebar/nav.tsx.packages/web/src/app/(app)/@sidebar/components/settingsSidebar/index.tsx— computeisOwner(same pattern asdefaultSidebar/index.tsx) and pass to<Nav>and<SidebarBase>.packages/web/src/app/(app)/@sidebar/components/defaultSidebar/nav.test.tsx— 3 vitest cases covering: (1) badge renders for an OWNER missing the entitlement, (2) badge is hidden for a non-owner MEMBER, (3) badge is hidden for an unauthenticated user.CHANGELOG.md— one-sentence entry under[Unreleased] → Fixed.Why this is in scope
The settings pages already gate on OWNER via
authenticatedPage(..., { minRole: OrgRole.OWNER, ... }), so the upsell cards inside/settings/securityand/settings/auditwere never affected — only the shared sidebar was missing the check. The fix lives in the sidebar, not in the entitlement-gated pages, because the sidebar is shared across all users.Why I didn't push the gate into
UpgradeBadgeitselfUpgradeBadgeis a tiny presentational element. Gating belongs in the parentNavso all callers (default and settings) get the same behaviour without each importing the auth context. The settings sidebar would otherwise need its owngetAuthContextcall just to render the badge.Test coverage
3 vitest cases in
nav.test.tsx:isOwnergate entirely is caught (the badge would still render for non-owners if the gate is removed, and the test suite would flip red).isOwnergate.isOwner = falsemeans a missing prop is treated the same as a MEMBER, which matches the production fallback path.The 7 pre-existing
getEnv is not a functionfailures inee/askmcp/...,ee/permissionSyncStatus/..., etc. are unchanged by this PR — they fail to load due to the OTel SDK version mismatch, before any of my code runs.Backward compatibility
Pure UI behaviour change. No API, no schema, no migration. Existing users on the OWNER role see no change. Non-owner users stop seeing prompts they couldn't act on.
Risks
Minimal. The settings pages already gate on OWNER, so the upsell cards inside
/settings/securityand/settings/auditare unaffected. The only behaviour change is in the shared sidebar, which is what the issue called out.Future work
hasEntitlement('org-management')check on the settings pages'UpgradeBadgeinstances is a related but separate concern — those pages are already owner-only, so the badge is only seen by owners, but a future "view-only settings" feature would need to gate them. Not in scope for this PR.<RoleGate role="OWNER">component would consolidate theisOwner && ...pattern across the sidebar and the settings pages. Punted; one-off gate is fine until we have three or more call sites.Note
Low Risk
UI-only visibility change with no API or auth logic changes; owners keep prior behavior.
Overview
Fixes#1524 by limiting sidebar upgrade CTAs to org owners, who are the ones who can act on billing.
An
isOwnerflag (fromgetAuthContext()+OrgRole.OWNER) is passed from the default and settings sidebar entry points intoSidebarBaseand bothNavimplementations. The footer Upgrade to Pro button now requiresisOwner && !isValidLicenseActive, and per-nav Upgrade badges requireisOwnerin addition to the existing missing-entitlement check. The default sidebar reuses owner detection it already had for settings notifications; the settings sidebar adds the same lookup.Vitest coverage was added for owner vs member vs unauthenticated badge/button visibility, plus a changelog entry under Unreleased → Fixed.
Reviewed by Cursor Bugbot for commit 680bfb8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation