Skip to content

Conversation

@kiram15
Copy link
Contributor

@kiram15 kiram15 commented Nov 3, 2025

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

actions={(
<div>
{renderActions() || renderDaysUntilPlanStartText('mt-4')}
{renderActions()}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why the renderDaysUntilPlanStartText might have been rendered twice in the first place? And it was breaking the tests so I took it out, but would love some context

@codecov
Copy link

codecov bot commented Nov 8, 2025

Codecov Report

❌ Patch coverage is 79.41176% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.03%. Comparing base (9f6d330) to head (a03dfbd).

Files with missing lines Patch % Lines
src/components/subscriptions/data/hooks.js 70.58% 10 Missing ⚠️
src/data/services/EnterpriseAccessApiService.ts 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1716      +/-   ##
==========================================
- Coverage   87.06%   87.03%   -0.04%     
==========================================
  Files         780      780              
  Lines       17766    17829      +63     
  Branches     3630     3733     +103     
==========================================
+ Hits        15468    15517      +49     
- Misses       2234     2237       +3     
- Partials       64       75      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kiram15 kiram15 marked this pull request as ready for review November 10, 2025 17:55
Copy link
Contributor

@iloveagent57 iloveagent57 left a comment

Choose a reason for hiding this comment

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

Looking good! One suggestion on that function signature

* @param {string} planType - The type of plan
* @param {func} setErrors - setErrors function from SubscriptionContext
*/
export const useStripeBillingPortalSession = ({ enterpriseUuid, planType, setErrors }) => {
Copy link
Contributor

@pwnage101 pwnage101 Nov 12, 2025

Choose a reason for hiding this comment

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

❌ Hm, so this whole pattern of a home-grown API hook with built-in state management is an anti-pattern that we've replaced with react useQuery. If Hamzah were still here reviewing frontend PRs I'm not sure we'd allow new code to use this pattern. One motivation to use react query in this instance is that the generated URL does eventually expire, but as written this hook won't refresh the URL after the first successful fetch. Using react query would allow us to leverage a built-in cache timeout setting which we can simply configure to a shorter duration.

Copy link
Contributor

@pwnage101 pwnage101 Nov 12, 2025

Choose a reason for hiding this comment

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

Oh wait, forgot this isn't the frontend-app-enterprise-checkout repo. Scratch what I said...

I'll re-state my concern: I think this hook does not gracefully handle when the generated URL expires.

Copy link
Contributor

@pwnage101 pwnage101 Nov 12, 2025

Choose a reason for hiding this comment

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

Expiration: 5 minutes after generation, or 1 hour if clicked.

https://docs.stripe.com/customer-management

Screenshot 2025-11-12 at 2 05 13 PM

Copy link
Contributor

@pwnage101 pwnage101 Nov 12, 2025

Choose a reason for hiding this comment

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

This is why I was originally surprised we didn't architect this endpoint to just be a link that itself performs a 302 redirect, so our frontend code didn't need to be bothered with implementing response parsing. Nevertheless, this is how the endpoint got implemented, so I'd just recommend we architect the button to directly perform an API call on click instead of using a hook to pre-cache the URL on render and retrieve it via hook state.

Comment on lines +81 to +83
key={stripeUrl}
as={Link}
to={stripeUrl}
Copy link
Contributor

@pwnage101 pwnage101 Nov 13, 2025

Choose a reason for hiding this comment

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

My suggestion is to make this button dynamic; here's an example in broad strokes:

         <Button
           id="Manage subscription button"
           onClick={handleManageSubscriptionClick}
           disabled={isGeneratingUrl}
           ...
         >

Then do dynamic stuff on click:

  const handleManageSubscriptionClick = async (e) => {
    const response = await EnterpriseAccessApiService.fetchStripeBillingPortalSession(
      enterpriseUuid
    );
    const results = camelCaseObject(response.data);
    if (results.url) {
      window.open(results.url, '_blank', 'noopener,noreferrer');
    }
  };

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.

4 participants