-
Notifications
You must be signed in to change notification settings - Fork 20
Feat/subscription #876
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
Feat/subscription #876
Conversation
WalkthroughAPI integration updates modify two user data fetching functions to be wallet-aware. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/consumer/MyFlexPlans/MyHostedPlan/MyHostedPlan.tsx (2)
132-135: Remove redundant empty string fallback.The guard on line 132 already ensures
accountis truthy before proceeding. Theaccount || ''fallback on line 135 is unnecessary and creates confusion about the expected value.Apply this diff to simplify:
const initSubscriptions = async () => { if (!account) return; try { setLoading(true); - const res = await getUserSubscriptions(account || ''); + const res = await getUserSubscriptions(account); if (!isConsumerHostError(res.data)) {
149-179: Consider adding an account guard for consistency.While
fetchHostingPlanswill only be called when subscriptions exist (which requiresaccount), adding an explicit guard would make the code more defensive and consistent withinitSubscriptions.Apply this diff:
const fetchHostingPlans = async (projectId: number) => { + if (!account) return; try { setExpandLoading(true); - const res = await getUserHostingPlansByProject(projectId, account || ''); + const res = await getUserHostingPlansByProject(projectId, account); if (!isConsumerHostError(res.data)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useConsumerHostServices.tsx(2 hunks)src/pages/consumer/MyFlexPlans/MyHostedPlan/MyHostedPlan.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/consumer/MyFlexPlans/MyHostedPlan/MyHostedPlan.tsx (1)
src/hooks/useConsumerHostServices.tsx (1)
isConsumerHostError(22-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/hooks/useConsumerHostServices.tsx (2)
510-522: Implementation looks good with query parameter approach.Using query parameters for the
walletparameter is a safer approach than path parameters, as it makes empty values more explicit (?wallet=) and easier for the backend to validate.
453-462: Verify backend validation and confirm address cannot be empty.The function has ONE call site at
src/pages/consumer/MyFlexPlans/MyHostedPlan/MyHostedPlan.tsx:135which callsgetUserSubscriptions(account || ''). While a guard condition (if (!account) return;) exists on line 132, the defensive fallback|| ''suggests the code expects empty addresses as possible. The endpoint directly injects this into the path without validation.Confirm:
- Does the backend validate and reject empty wallet addresses?
- Can
accountever be an empty string despite the guard condition?- Is the
|| ''fallback necessary, or can it be removed for clarity?
| const res = await getUserSubscriptions(); | ||
| const res = await getUserSubscriptions(account || ''); | ||
| if (!isConsumerHostError(res.data)) { | ||
| console.warn(res.data); |
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.
Remove debug logging statement.
The console.warn statement appears to be leftover debug code and should be removed before merging to production.
Apply this diff:
const res = await getUserSubscriptions(account);
if (!isConsumerHostError(res.data)) {
- console.warn(res.data);
setSubscriptions(res.data);
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.warn(res.data); | |
| const res = await getUserSubscriptions(account); | |
| if (!isConsumerHostError(res.data)) { | |
| setSubscriptions(res.data); | |
| } else { |
🤖 Prompt for AI Agents
In src/pages/consumer/MyFlexPlans/MyHostedPlan/MyHostedPlan.tsx around line 137,
remove the leftover debug logging statement `console.warn(res.data);` so no
debug console output remains in production; delete that line (or replace with
proper user-facing/error handling logic if needed) and run lint/tests to ensure
nothing else depends on it.
Description
Type of change
Please delete options that are not relevant.
Summary by CodeRabbit