-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stripe #413
Stripe #413
Conversation
.service( | ||
web::scope("api/v1/auth") | ||
.wrap(shared_secret_auth.clone()) | ||
.service(routes::auth::signin), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was duplicated (right above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to b19d639 in 2 minutes and 24 seconds
More details
- Looked at
728
lines of code in14
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. frontend/lib/checkout/utils.ts:75
- Draft comment:
Ensure SQL fragment with CASE uses proper parameterization to avoid injection risks. Consider refactoring for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code is actually using proper parameterization through drizzle-orm's sql template literals. The CASE statement structure is clear and follows standard SQL patterns. The comment appears to be raising a false concern since the code is already following security best practices.
I could be wrong about drizzle-orm's sql template literal safety guarantees. There might be edge cases I'm not considering.
Drizzle-orm is a well-established ORM and its sql template literal feature is specifically designed to handle SQL injection protection. The usage here follows the documented patterns.
The comment should be deleted as it raises concerns about SQL injection that are already properly addressed by the code's use of drizzle-orm's safe SQL templating.
2. frontend/app/checkout/page.tsx:35
- Draft comment:
Using non-null assertion (!) on STRIPE_SECRET_KEY may cause runtime errors if not set. Consider a safer check. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/app/webhook/route.ts:100
- Draft comment:
Guard against a missing 'stripe-signature' header before using it. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/lib/features/features.ts:36
- Draft comment:
Feature flag logic is clear; consider handling undefined ENVIRONMENT explicitly for consistency. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. frontend/app/webhook/route.ts:141
- Draft comment:
The async calls to handleSubscriptionChange in the 'customer.subscription.created' and 'customer.subscription.updated' cases are not awaited. Consider awaiting these calls to ensure errors are properly caught. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/lib/checkout/utils.ts:136
- Draft comment:
Review the SQL expression in updateUsageCacheForWorkspace. Verify that the column names (e.g. 'span_count', 'spans', and 'name') correctly match your schema and that the logic for checking if the 'free' tier limit is exceeded works as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify the column names and logic, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue.
7. frontend/lib/checkout/utils.ts:74
- Draft comment:
Ensure that the inline SQL CASE statement for updating tierId in manageSubscriptionEvent properly handles parameterization. Confirm that using the sql tagged template here safely interpolates the boolean and productId values and that the intended logic (setting tierId to 1 when cancel is true) is correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%
<= threshold50%
The comment is asking the author to confirm the safety and correctness of the SQL statement, which is not allowed. However, it also points out a specific concern about parameterization and logic, which could be useful. The comment could be rephrased to focus on the specific suggestion or concern without asking for confirmation.
8. app-server/src/routes/mod.rs:28
- Draft comment:
Consider capitalizing the first word in the doc comment ("returns") to "Returns" for consistency with other comments. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_i6WWA5kd15Gm5OwG
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
frontend/app/checkout/page.tsx
Outdated
const userId = existingStripeCustomer?.userId ?? | ||
(await db.query.users.findFirst({ | ||
where: eq(users.email, userSession.user.email!) | ||
}))!.id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the non-null assertion (!) on the result of db.query.users.findFirst might panic if the user isn’t found. Consider handling the null case explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 64cbeae in 1 minute and 42 seconds
More details
- Looked at
32
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/app/checkout/page.tsx:45
- Draft comment:
Good improvement adding an explicit check for userId after the DB query to prevent potential runtime errors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it praises the addition of an explicit check without suggesting any changes or improvements. It does not align with the rules for useful comments.
2. frontend/app/checkout/page.tsx:8
- Draft comment:
Minor: Import ordering change. Consider consistent ordering (e.g., alphabetical) for clarity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. frontend/app/checkout/page.tsx:45
- Draft comment:
Good use of optional chaining instead of the non-null assertion. However, note the redirect usage is inconsistent—sometimes called directly, sometimes with a return. Clarify whether redirect always halts execution (or consistently use return redirect(...)) for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/app/checkout/page.tsx:1
- Draft comment:
The import order has been adjusted. Ensure the reordering aligns with project conventions for consistency. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
Workflow ID: wflow_w5B6DhWARsAHWq89
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 91b2227 in 41 seconds
More details
- Looked at
32
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/app/checkout/page.tsx:1
- Draft comment:
Redundant reordering of imports. Ensure import order consistency across the project. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. frontend/lib/checkout/utils.ts:10
- Draft comment:
Minor whitespace cleanup in comment; consider consistent formatting for clarity. - Reason this comment was not posted:
Confidence changes required:10%
<= threshold50%
None
3. frontend/app/checkout/page.tsx:1
- Draft comment:
Reordering imports for clarity; ensure consistent grouping of external and local imports. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. frontend/lib/checkout/utils.ts:117
- Draft comment:
Minor cleanup: removed extra space in comment. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_7Qvdhwe4ZCYpLwo2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Remove backend subscription management and refactor frontend to handle subscriptions directly with Stripe and database interactions.
subscriptions.rs
fromdb
androutes
.main.rs
.Feature::Subscription
fromfeatures/mod.rs
.CheckoutPage
andCheckoutPortalPage
incheckout
to usegetUserSubscriptionInfo
and direct Stripe API calls.utils.ts
.PurchaseSeatsDialog
to post directly to/api/workspaces/{workspaceId}/update-seats
.Feature::SUBSCRIPTION
tofeatures.ts
in frontend.route.ts
to manage subscription events directly.This description was created by
for 91b2227. It will automatically update as commits are pushed.