Subscription client changes #424
Conversation
|
Brandon says: |
brandonkachen
left a comment
brandonkachen
left a comment
There was a problem hiding this comment.
mostly looks good, just needs some de-slopification. one key issue to fix/consciously ignore before merging: https://github.com/CodebuffAI/codebuff/pull/424/changes#r2748507503
| const isSubscriptionLimited = subscriptionRateLimit?.limited === true | ||
|
|
||
| // Get subscription reset time | ||
| const subscriptionResetTime = subscriptionRateLimit |
There was a problem hiding this comment.
not a fan of all these nested ternaries, would be worthwhile (imo) to ask codebuff to replace them all with reasonably named constants and if blocks or, more ideally, states that we care about and reason about more clearly
| @@ -1 +1 @@ | |||
| import React from 'react' | |||
There was a problem hiding this comment.
just a personal preference thing, but i really don't care for bottom status bars. they take up a lot of vertical space (esp right now, since ours only shows subscriptions) and the info they show can still make users feel like they have to ration usage.
i don't mind that claude code/codex/kimi code hide this info in the cli and you have to go to the website to see it
| </box> | ||
| )} | ||
|
|
||
| {/* Show Codebuff Strong when subscribed and Claude not healthy */} |
There was a problem hiding this comment.
shouldn't we display both at the same time? if a user has both subs active, they may want to see how both are being used (esp if we use more non-claude models in the future). again, i'm still against the need for this component in the first place
| ⏱️ Block limit reached | ||
| </text> | ||
| <text style={{ fg: theme.muted }}> | ||
| You've used all {rateLimit.blockLimit?.toLocaleString()} credits in this 5-hour block. |
There was a problem hiding this comment.
nit: we should use the constant for the block size.
There was a problem hiding this comment.
also, if rateLimit.blockLimit doesn't exist, we should put a placeholder string, e.g. "your" -> "You've used all credits..."
| <> | ||
| <Button onClick={handleContinueWithCredits}> | ||
| <text style={{ fg: theme.foreground }}> | ||
| [Continue with a-la-carte] ({remainingBalance.toLocaleString()} available) |
There was a problem hiding this comment.
should we make these look like proper buttons? like our /feedback submit button?
| <ProgressBar | ||
| percentAvailable={ | ||
| rateLimit.blockLimit != null && rateLimit.blockUsed != null && rateLimit.blockLimit > 0 | ||
| ? 100 - (rateLimit.blockUsed / rateLimit.blockLimit) * 100 |
There was a problem hiding this comment.
wait we shouldn't pass back the actual amounts from the server to the client; only the server should know how much is actually used, the client should only ever see percentages
| Upgrade to {SUBSCRIPTION_DISPLAY_NAME} | ||
| </h3> | ||
| <p className="text-sm text-muted-foreground"> | ||
| From $100/mo · Work in focused 5-hour sessions with no |
There was a problem hiding this comment.
we're not releasing tiers yet, right? so this number should be the tier we actually want to show users. and if we're gonna hardcode something, it should be the $200/mo plan.
| const canonicalUrl = `${env.NEXT_PUBLIC_CODEBUFF_APP_URL}/strong` | ||
| const title = `Codebuff ${SUBSCRIPTION_DISPLAY_NAME} — The Strongest Coding Agent` | ||
| const description = | ||
| 'Deep thinking, multi-agent orchestration, and the strongest coding agent. Plans from $100/mo.' |
There was a problem hiding this comment.
hardcoded $100 again
| .update(schema.subscription) | ||
| .set({ | ||
| scheduled_tier: scheduledTier, | ||
| updated_at: new Date(), |
There was a problem hiding this comment.
it would be nice to build updated_at on the postgres layer, instead of having to set it each time like this. i think we'd have to create a new migration and write sql code, but that's how it works for most/all other updated_at fields 🤷♂️
| } | ||
|
|
||
| // Need at least 2 phases to have a scheduled change (current + future) | ||
| if (!schedule.phases || schedule.phases.length < 2) { |
There was a problem hiding this comment.
we should definitely test to make sure we always get 2+ phases. i'm not totally sure what that is
No description provided.