Skip to content

Conversation

@MaheshtheDev
Copy link
Member

@MaheshtheDev MaheshtheDev commented Oct 3, 2025

Before we are showing 0/0

image.png

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 3, 2025

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
supermemory-app aa25b37 Oct 03 2025, 07:58 AM

Copy link
Member Author


How to use the Graphite Merge Queue

Add the label Main to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@MaheshtheDev MaheshtheDev marked this pull request as ready for review October 3, 2025 04:00
@graphite-app graphite-app bot requested a review from Dhravya October 3, 2025 04:00
@claude
Copy link

claude bot commented Oct 3, 2025

Code Review: Fix billing metrics display for pro users

Summary

This PR addresses the display issue where Pro users were seeing "0/0" for memory metrics. The changes correctly implement "Unlimited" display for Pro users while maintaining the progress bar for Free users.

Positive Aspects

  1. Clean Solution: The fix is minimal and targeted, addressing exactly what was needed without unnecessary refactoring.

  2. Consistent UX: Both billing.tsx and profile.tsx now consistently show "Unlimited" for Pro users, providing a better user experience.

  3. Proper Conditional Rendering: The use of conditional rendering correctly shows/hides UI elements based on subscription status.

Issues and Concerns

1. Logic Issue in profile.tsx:210-214

There is a conditional inside the progress bar that will never execute. The code checks isPro inside a block that is wrapped with {!isPro && ...} at line 206, so isPro will always be false inside this block. The isPro check for bg-green-500 is unreachable dead code.

Recommendation: Simplify the className to only check memoriesUsed >= memoriesLimit for red vs blue.

Location: apps/web/components/views/profile.tsx:210-214

2. Inconsistent Import Paths

billing.tsx uses @ui/components/button while profile.tsx uses @repo/ui/components/button. Consider standardizing import paths across components.

Testing Considerations

Test scenarios to verify:

  1. Pro users see "Unlimited" with no progress bar
  2. Free users see "X / Y" with progress bar
  3. Edge cases with undefined/null values are handled

Performance

No performance concerns. The changes are purely presentational.

Security

No security concerns identified.

Suggestions for Future Enhancement

  1. Type Safety: Consider defining a type/interface for plan features to make Pro/Free differences more explicit.

  2. DRY Principle: The billing display logic is duplicated between billing.tsx and profile.tsx. Consider extracting to a shared component.

Overall Assessment

Approved with minor suggestions. The core fix works well and solves the reported issue. The dead code issue in profile.tsx:210-214 should be cleaned up, but it does not affect functionality since that code path is unreachable.

Recommendation: Merge after removing the unreachable isPro check in the progress bar styling.

@graphite-app
Copy link

graphite-app bot commented Oct 3, 2025

Merge activity

@graphite-app
Copy link

graphite-app bot commented Oct 3, 2025

TV gif. Steve Irwin the Crocodile Hunter looking down at a body of water, turns around and gives a double thumbs-up, mouthing 'that's good.' (Added via Giphy)

@claude
Copy link

claude bot commented Oct 3, 2025

Code Review: Fix billing metrics display for pro users

✅ Positive Changes

1. Correct Business Logic
The fix properly addresses the issue where Pro users were seeing 0/0 for memory limits. Displaying "Unlimited" is the right UX decision for Pro users.

2. Consistent Implementation
The changes are applied correctly in both billing.tsx and profile.tsx, maintaining consistency across the application.

3. Clean Conditional Rendering
The use of conditional rendering ({isPro ? ... : ...} and {!isPro && ...}) is clear and follows React best practices.


🐛 Potential Issues

1. Missing Update in billing.tsx (Line 121)
After your changes, the Pro section in billing.tsx still shows numeric limits:

<span className="text-sm text-foreground">
  {memoriesUsed} / {memoriesLimit}
</span>

This should also display "Unlimited" for consistency. The progress bar was removed (good), but the text still shows the numeric limit.

Suggested fix:

<span className="text-sm text-foreground">
  Unlimited
</span>

2. Inconsistent Logic in profile.tsx (Lines 204-205)
Inside the progress bar rendering logic, there's a check for isPro even though the entire progress bar is wrapped in {!isPro && ...}:

{!isPro && (
  <div className="w-full bg-muted-foreground/50 rounded-full h-2">
    <div
      className={`h-2 rounded-full transition-all ${
        memoriesUsed >= memoriesLimit
          ? "bg-red-500"
          : isPro  // ← This condition will always be false here
            ? "bg-green-500"
            : "bg-blue-500"
      }`}

Since this is inside {!isPro && ...}, the isPro check in the ternary will always be false. This means the green color will never be used. This appears to be dead code.

Suggested fix:

{!isPro && (
  <div className="w-full bg-muted-foreground/50 rounded-full h-2">
    <div
      className={`h-2 rounded-full transition-all ${
        memoriesUsed >= memoriesLimit ? "bg-red-500" : "bg-blue-500"
      }`}

3. Division by Zero Risk
When isPro is true, memoriesLimit might be 0 (as indicated in the original issue). In the billing view, if you keep the numeric display or add it back, the calculation (memoriesUsed / memoriesLimit) * 100 will produce Infinity when memoriesLimit is 0.

While this is now avoided by removing the progress bar, it's worth noting for the remaining instance in billing.tsx.


🎨 Code Quality

Formatting: ✅ Good - The code follows the project's Biome formatting standards
Type Safety: ✅ Good - No type issues introduced
Component Structure: ✅ Good - Changes maintain existing component structure


🧪 Test Coverage

Missing: No test coverage for this change. Consider adding tests for:

  • Pro users see "Unlimited" text
  • Free users see numeric limits X / Y
  • Progress bar only renders for non-Pro users
  • Edge cases: memoriesLimit === 0, memoriesUsed > memoriesLimit

📊 Performance

No performance concerns - the changes are simple conditional renders.


🔒 Security

No security concerns identified.


📝 Summary

The PR successfully fixes the 0/0 display issue for Pro users. However, there are two issues to address:

  1. Critical: The Pro section in billing.tsx (line 121) still shows numeric limits instead of "Unlimited"
  2. Minor: Dead code in profile.tsx - the isPro check inside the {!isPro && ...} block will never be true

Recommendation: Address issue #1 before merging, as it means the bug is only partially fixed. Issue #2 is a minor cleanup that can be done now or later.

@graphite-app graphite-app bot force-pushed the 10-02-fix_billing_metrics_display_for_pro_users branch from 4823d1f to aa25b37 Compare October 3, 2025 07:53
@claude
Copy link

claude bot commented Oct 3, 2025

Pull Request Review: Fix billing metrics display for pro users

Summary

This PR successfully addresses the issue where Pro users were seeing "0/0" for their memory usage. The fix appropriately displays "Unlimited" for Pro users while maintaining the existing usage display for Free users.


Positive Aspects

  1. Clean Solution: The conditional rendering approach is straightforward and maintainable
  2. Consistent UX: Both billing.tsx and profile.tsx now consistently show "Unlimited" for Pro users
  3. Appropriate Simplification: Removing the progress bar for Pro users makes sense since there's no limit to track

🐛 Potential Issues

Critical: Redundant Code in profile.tsx (line 212-214)

className={`h-2 rounded-full transition-all ${
  memoriesUsed >= memoriesLimit
    ? "bg-red-500"
    : isPro
      ? "bg-green-500"  // ⚠️ This condition is unreachable
      : "bg-blue-500"
}`}

Issue: The progress bar is only rendered when !isPro (line 206), but the ternary inside still checks isPro. This condition will never be true since the parent condition ensures isPro === false.

Fix: Remove the redundant check:

className={`h-2 rounded-full transition-all ${
  memoriesUsed >= memoriesLimit ? "bg-red-500" : "bg-blue-500"
}`}

Location: apps/web/components/views/profile.tsx:209-214


🎯 Code Quality Improvements

1. Type Safety (apps/web/components/views/profile.tsx:33-38)

Consider adding explicit types for the feature check data:

type MemoriesFeature = {
  usage: number;
  included_usage: number;
} | undefined;

const { data: memoriesCheck }: { data: MemoriesFeature } = fetchMemoriesFeature(
  autumn,
  !isCheckingStatus && !autumn.isLoading,
);

2. Magic Numbers (apps/web/components/views/billing.tsx:129)

The connection limit 10 appears hardcoded. Consider:

const CONNECTIONS_LIMIT = 10;
// or fetch from connectionsCheck?.included_usage if available

Performance Considerations

No performance concerns identified. The changes are purely presentational with minimal runtime impact.


🔒 Security Concerns

No security issues identified. The changes don't introduce any new attack vectors.


🧪 Test Coverage

Missing: No tests found for these components.

Recommendation: Add unit tests to verify:

  • Pro users see "Unlimited" text
  • Free users see usage ratio (e.g., "150 / 200")
  • Progress bar is hidden for Pro users
  • Progress bar displays correctly for Free users
  • Color coding works (red when limit exceeded, blue otherwise)

Example test structure:

describe('ProfileView', () => {
  it('shows unlimited for pro users', () => {
    // Mock isPro = true
    // Assert "Unlimited" is rendered
    // Assert progress bar is not rendered
  });
  
  it('shows usage metrics for free users', () => {
    // Mock isPro = false, usage data
    // Assert "X / Y" format is rendered
    // Assert progress bar is rendered
  });
});

📝 Additional Observations

  1. Consistency: The billing.tsx file also hardcodes "10" for connections (line 129). This should ideally come from the API or a shared constant.

  2. Error Handling: Both components use ?? 0 for fallback values, which is good defensive programming.

  3. Formatting: The changes follow the existing code style and Biome formatting conventions.


Recommendation

Approve with minor fixes requested

The PR solves the stated problem effectively. However, please address the redundant isPro check in profile.tsx:209-214 before merging, as it creates confusion and technical debt.


📋 Action Items

  • Remove redundant isPro ternary check in profile.tsx:212
  • Consider extracting connection limit magic number to constant
  • Add unit tests for Pro vs Free user display logic (future PR)

@graphite-app graphite-app bot merged commit aa25b37 into main Oct 3, 2025
3 of 4 checks passed
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.

3 participants