-
Notifications
You must be signed in to change notification settings - Fork 44
feat(session): spending limit step for register session #2303
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| setError(e as Error); | ||
| } finally { | ||
| setIsConnecting(false); | ||
| } |
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.
Registration error swallowing prevents display in ExecutionContainer
The onRegisterSession function catches errors but does not re-throw them, causing the ExecutionContainer to assume the operation succeeded. Consequently, errors occurring during registration are swallowed and not displayed to the user on the summary page, as the local error state is only consumed by the non-rendered SpendingLimitPage.
Additional Locations (1)
| if (hasTokenApprovals && step === "spending-limit") { | ||
| setStep("summary"); | ||
| } | ||
| }, [hasTokenApprovals, step]); |
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.
Confirm and Back buttons perform identical actions
Medium Severity
The handleSpendingLimitConfirm callback passed to SpendingLimitPage as onConnect only calls setStep("summary"), which is exactly the same action as the onBack callback. Both the "Confirm" and "Back" buttons result in identical navigation to the summary step. In contrast, CreateSession.tsx correctly calls handlePrimaryAction() for onConnect, which triggers the actual session creation. The isConnecting and error props passed to SpendingLimitPage will also never reflect meaningful state since registration only occurs after leaving this page.
🔬 Verification Test
Why verification test was not possible: This is a React component logic bug that requires UI interaction to demonstrate. The bug can be verified by code inspection: both onBack={() => setStep("summary")} (line 155) and handleSpendingLimitConfirm (which calls setStep("summary") at lines 138-142) result in the same state transition. Comparing with CreateSession.tsx lines 195-197 shows onConnect should call the actual action handler (handlePrimaryAction), not just navigate to another step.
Additional Locations (1)
| setStep("summary"); | ||
| } | ||
| }, [hasTokenApprovals, step]); | ||
|
|
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.
SpendingLimitPage unnecessarily blocked by transactions loading check
Low Severity
The if (!transactions) check at line 144 returns "Loading" before the SpendingLimitPage check at line 148. However, SpendingLimitPage doesn't use transactions at all—it only needs policies from context. This causes users to see an unnecessary loading state while transactions are being prepared, when SpendingLimitPage could display immediately. The SpendingLimitPage conditional should be checked before the transactions loading check to avoid this unnecessary delay. CreateSession.tsx handles this correctly by not blocking on transactions.
🔬 Verification Test
Why verification test was not possible: This is a React component render order issue that manifests as a UX delay. The bug can be verified by code inspection: SpendingLimitPage (line 151) only receives policies, isConnecting, error, onBack, and onConnect as props—none of which depend on transactions. Yet the if (!transactions) return <div>Loading</div> check (line 144) executes first, blocking SpendingLimitPage from rendering until transactions load. Comparing with CreateSession.tsx lines 184-199 shows the correct pattern where SpendingLimitPage is rendered without waiting for transactions.
Note
Introduces a spending limit flow in
RegisterSessionand improves transaction handling.SpendingLimitPageand step logic to show a spending-limit step by default for verified sessions with token approvals (hasApprovalPolicies)isConnectinganderrorstate with try/catch/finally aroundregisterSession, passing state toSpendingLimitPage.tool-versionsto Node.js20.19.5Written by Cursor Bugbot for commit bfe4b75. This will update automatically on new commits. Configure here.