Skip to content

feat(web): conditionally show upgrade toast only to org owners#1024

Open
ksg98 wants to merge 1 commit intosourcebot-dev:mainfrom
ksg98:ksg98/fix-upgrade-toast-owner-only
Open

feat(web): conditionally show upgrade toast only to org owners#1024
ksg98 wants to merge 1 commit intosourcebot-dev:mainfrom
ksg98:ksg98/fix-upgrade-toast-owner-only

Conversation

@ksg98
Copy link

@ksg98 ksg98 commented Mar 22, 2026

Summary

  • Only show the upgrade version toast to org owners, not regular members or anonymous users
  • Added isOwner prop to UpgradeToast component with early return guard in the useEffect
  • Added enabled: isOwner to the useQuery call to skip the version API request entirely for non-owners
  • Hoisted the membership query in layout.tsx so the role is available at render time
  • Added unit tests for version utilities and the isOwner gating behavior

Test plan

  • All 272 existing tests pass (12 test files)
  • New upgradeToast.test.tsx tests verify:
    • Version string parsing (valid, invalid, zero)
    • Version formatting
    • Version comparison (major, minor, patch)
    • Toast is NOT shown and fetch is NOT called when isOwner=false
    • GitHub tags ARE fetched when isOwner=true

Fixes #815

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Upgrade notifications now appear only to workspace owners, preventing non-owners from triggering upgrade checks.
  • Tests

    • Added comprehensive test coverage for upgrade notifications, including version comparison utilities and permission gating behavior.

Only org owners should see the upgrade notification toast. Non-owners
and anonymous users no longer trigger the version check or see the popup.

Fixes sourcebot-dev#815

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 22, 2026

Walkthrough

The changes implement owner-only gating for the UpgradeToast component by adding an isOwner prop that conditionally disables version checking and toast display, with corresponding test coverage and layout refactoring to derive and pass owner status based on membership role.

Changes

Cohort / File(s) Summary
Test Coverage
packages/web/src/app/[domain]/components/upgradeToast.test.tsx
New test suite covering utility functions (getVersionFromString, getVersionString, compareVersions) and UpgradeToast component behavior with isOwner prop gating, including assertions for fetch suppression when isOwner={false} and GitHub tags request when isOwner={true}.
UpgradeToast Component
packages/web/src/app/[domain]/components/upgradeToast.tsx
Added isOwner prop to component signature via new UpgradeToastProps interface; React Query request now conditionally disabled via enabled: isOwner; useEffect early-returns when !isOwner to prevent version parsing, fetch, and toast triggering; helper functions getVersionFromString, getVersionString, compareVersions now exported.
Layout Integration
packages/web/src/app/[domain]/layout.tsx
Refactored membership to always be defined (null when unauthenticated) using conditional expression; added isOwner prop passed to UpgradeToast derived from membership?.role === OrgRole.OWNER; imported OrgRole enum from @sourcebot/db.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • brendan-kellam
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: conditionally displaying the upgrade toast only to organization owners.
Linked Issues check ✅ Passed The PR fully addresses issue #815 by implementing role-based gating that restricts the upgrade toast to users with the owner role through the isOwner prop and useEffect guard.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing the owner-only upgrade toast feature: isOwner prop, conditional logic, membership query hoisting, and comprehensive unit tests.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can suggest fixes for GitHub Check annotations.

Configure the reviews.tools.github-checks setting to adjust the time to wait for GitHub Checks to complete.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/web/src/app/[domain]/components/upgradeToast.test.tsx (1)

75-77: Mock doesn't respect enabled option, but test still validates correctly.

The useQuery mock always returns { data: 'v1.0.0' } regardless of the enabled option. In reality, when enabled: false, react-query returns undefined for data. The test still passes because the component's useEffect has an early return for !isOwner, so this doesn't cause a false positive.

Consider making the mock more realistic for future-proofing:

♻️ Proposed improvement for mock fidelity
 vi.mock('@tanstack/react-query', () => ({
-    useQuery: () => ({ data: 'v1.0.0' }),
+    useQuery: ({ enabled = true }: { enabled?: boolean }) => ({
+        data: enabled ? 'v1.0.0' : undefined,
+    }),
 }));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/web/src/app/`[domain]/components/upgradeToast.test.tsx around lines
75 - 77, The useQuery mock in the test is unrealistic because it always returns
{ data: 'v1.0.0' } even when the react-query `enabled` option is false; update
the vi.mock for '@tanstack/react-query' so the mocked useQuery inspects the
passed options (e.g., `options.enabled`) and returns { data: 'v1.0.0' } only
when enabled is true/undefined, otherwise returns { data: undefined } to mirror
real react-query behavior; target the mocked useQuery implementation in the test
(the vi.mock block) and ensure it accepts the same args signature so tests that
rely on `enabled` behave correctly (the component's useEffect and isOwner
branches will then be validated realistically).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/web/src/app/`[domain]/components/upgradeToast.test.tsx:
- Around line 75-77: The useQuery mock in the test is unrealistic because it
always returns { data: 'v1.0.0' } even when the react-query `enabled` option is
false; update the vi.mock for '@tanstack/react-query' so the mocked useQuery
inspects the passed options (e.g., `options.enabled`) and returns { data:
'v1.0.0' } only when enabled is true/undefined, otherwise returns { data:
undefined } to mirror real react-query behavior; target the mocked useQuery
implementation in the test (the vi.mock block) and ensure it accepts the same
args signature so tests that rely on `enabled` behave correctly (the component's
useEffect and isOwner branches will then be validated realistically).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1a16f3b9-9f85-4408-866a-dde0fcd487d6

📥 Commits

Reviewing files that changed from the base of the PR and between e0d4ff9 and 94eb598.

📒 Files selected for processing (3)
  • packages/web/src/app/[domain]/components/upgradeToast.test.tsx
  • packages/web/src/app/[domain]/components/upgradeToast.tsx
  • packages/web/src/app/[domain]/layout.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FR] Upgrade pop-up should only be shown to current owner, not users

1 participant