Conversation
|
Thanks @Rodriguespn for taking on this one. Let me know when you want to have a first review to discuss the API and the implementation ;) |
Hey @fredericbarthelet, I've marked the PR as a review because I didn't have the time to properly test it on chatgpt. The implementation seems to be ready to be tested (it's vibe coded so I would proceed with caution). Will try to find some time between today and tomorrow to test this on chatgpt but if you find the time yourself feel free to test it on your end. |
05e3525 to
faa9ba7
Compare
There was a problem hiding this comment.
Additional Comments (1)
-
src/web/hooks/use-checkout.ts, line 155-163 (link)logic: double state update when
requestCheckoutreturns an error response - state is set here (lines 157-161) then thrown and caught by the catch block below which sets state again (lines 179-183)this causes redundant
setCheckoutStatecalls. consider removing the throw on line 162 and just returning early after setting the error state, or remove the state update here and let the catch block handle itNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
5 files reviewed, 1 comment
9ce2143 to
17007c5
Compare
|
@fredericbarthelet tested and ready to be reviewed. I've also added a examples package with a very simple checkout app using Skybridge and Stripe based on an app made by xmcp. Let me know if you guys want to keep it. Happy to remove it if not 😄 |
1d7666c to
c781f67
Compare
|
Hey @fredericbarthelet @qchuchu, just checking if this feature still makes sense to be in Skybridge. Happy to steer this in the right direction. |
|
Hey @Rodriguespn, this feature still makes sense. Sorry for not reviewing this earlier. Adapting for MCP App compatibility took a bit of bandwitdth. I've red your implementations of I need to ask for a few changes in order to make to move this forward:
WDYT ? |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement Fred's suggestions #2 and alpic-ai#3: **Code Consolidation (#2):** - Create shared useAsyncOperation base hook - Extract common async state management patterns - Refactor useCheckout to use base hook - Refactor useCallTool to use base hook with deduplication - Eliminate ~80-100 lines of duplicated code **Checkout-Specific Features (alpic-ai#3):** - Add automatic session ID generation with crypto.randomUUID() - Add UseCheckoutOptions for custom session ID generator - Expose sessionId directly in useCheckout return value - Expose order directly in useCheckout return value - Maintain error discrimination with isCheckoutErrorResponse **Tests:** - Add 7 new tests for session ID generation and order access - All 95 tests passing (up from 88) - Full backward compatibility maintained 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
c781f67 to
8605a5d
Compare
|
Hey @fred! Thanks for the feedback and patience. I've addressed all your review points you mentioned besides the server tool implementation for ACP implementation:
Removed the example app since is out of scope, removed in-file comments and moved them to proper docs, consolidated the shared codebase with the new base hook, and implemented the checkout-specific features you mentioned. Regarding the ACP implementation for OpenAI's server tool, happy to tackle that in a separate PR later as you suggested. |
|
|
||
| ```tsx | ||
| const { | ||
| data, |
There was a problem hiding this comment.
Do you still expose data in addition to order? I feel like this might be redundant
| {isPending ? "Processing..." : "Checkout"} | ||
| </button> | ||
| {sessionId && <p>Session ID: {sessionId}</p>} | ||
| {isSuccess && order && ( |
There was a problem hiding this comment.
| {isSuccess && order && ( | |
| {isSuccess && ( |
I feel like isSuccess should be sufficient for order to be defined
| ); | ||
|
|
||
| const { build, preview, ...devConfig } = configResult?.config || {}; | ||
| const { ...devConfig } = configResult?.config || {}; |
There was a problem hiding this comment.
I think you might have a conflicting version of biome on your IDE (or at least different that the one being used on this project). We experienced a few issues recently on that front: the extreme step of re downloading the project from Github did the trick for most of the team.
Anyhow, this should not be part of this PR
| let resolveOperation: (value: string) => void; | ||
| const operationPromise = new Promise<string>((resolve) => { | ||
| resolveOperation = resolve; | ||
| }); |
There was a problem hiding this comment.
You have a very convenient Promise syntax to create such "to be resolved" promise: Promise.withResolvers
| export type UseAsyncOperationConfig = { | ||
| enableDeduplication?: boolean; | ||
| }; |
There was a problem hiding this comment.
All the implementations of this hook seem to enable deduplication. I feel like this config parameter is not useful
| error: TError; | ||
| }; | ||
|
|
||
| export type AsyncOperationState<TData, TError = unknown> = |
There was a problem hiding this comment.
There is not a single location where AsyncOperationState is defined without its generic types, so you can drop the default generic value
| export type AsyncOperationState<TData, TError = unknown> = | |
| export type AsyncOperationState<TData, TError> = |
|
@greptile could you review again? |
| const execute = async ( | ||
| toolArgs: ToolArgs, | ||
| ): Promise<CombinedCallToolResponse> => { | ||
| const callId = ++callIdRef.current; | ||
| setCallToolState({ status: "pending", data: undefined, error: undefined }); | ||
|
|
||
| try { | ||
| const data = await adaptor.callTool<ToolArgs, CombinedCallToolResponse>( | ||
| return executeAsync(async () => { | ||
| return adaptor.callTool<ToolArgs, CombinedCallToolResponse>( | ||
| name, | ||
| toolArgs, | ||
| ); | ||
| if (callId === callIdRef.current) { | ||
| setCallToolState({ status: "success", data, error: undefined }); | ||
| } | ||
|
|
||
| return data; | ||
| } catch (error) { | ||
| if (callId === callIdRef.current) { | ||
| setCallToolState({ status: "error", data: undefined, error }); | ||
| } | ||
| throw error; | ||
| } | ||
| }); |
There was a problem hiding this comment.
why not use execute returned by useAsyncOperation directly and instead redefining a monad here?
There was a problem hiding this comment.
Maybe it will be easier to understand if useAsyncOperation takes in the async callback as argument instead?
const { state, execute } = useAsyncOperation<
CombinedCallToolResponse,
unknown
>((toolArgs) => adaptor.callTool(name, toolArgs));| * Checkout session request payload following the ACP specification. | ||
| */ | ||
| export type CheckoutSessionRequest = { | ||
| id: string; |
There was a problem hiding this comment.
The id field is marked as required, but useCheckout hook has logic to auto-generate IDs when not provided (line 83-85 in use-checkout.ts). This creates a type inconsistency where the hook expects id to be optional but the type declares it as required.
Consider making the field optional to match the actual behavior:
| id: string; | |
| id?: string; |
This would align the type with the hook's auto-injection feature and eliminate the need for type assertions in tests (see line 332 in use-checkout.test.ts).
| ? session | ||
| : { ...session, id: generateSessionId() }; | ||
|
|
||
| setSessionId(sessionWithId.id); |
There was a problem hiding this comment.
setSessionId is called inside the async operation, which can lead to state inconsistency when deduplication is enabled. If a second checkout is initiated before the first completes, both will update sessionId, but only the second operation's result will be reflected in the state. This means sessionId could reference a session whose result was discarded due to deduplication.
| ); | ||
|
|
||
| const { build, preview, ...devConfig } = configResult?.config || {}; | ||
| const { ...devConfig } = configResult?.config || {}; |
There was a problem hiding this comment.
Removing build and preview from destructuring means these properties will now be passed to createServer via ...devConfig. While Vite likely ignores unknown properties, this unrelated change wasn't mentioned in the PR description and could introduce unexpected behavior.
| return execute(session); | ||
| }; | ||
|
|
||
| const requestCheckout: RequestCheckoutFn = (( |
There was a problem hiding this comment.
can't this be part of the useAsyncOperation as well? I feel like it's duplicated as well
fredericbarthelet
left a comment
There was a problem hiding this comment.
Hey @Rodriguespn thanks for your contribution. It's close to be ready!
I just shared a few comments to improve tests suite and useAsyncOperation hook. Let me know what you think of it
Greptile Overview
Greptile Summary
This PR adds Instant Checkout support for ChatGPT apps through a new
useCheckouthook, following the ACP specification. The implementation includes comprehensive TypeScript types, thorough test coverage, and good documentation.Major changes:
useCheckouthook for payment processing withwindow.openai.requestCheckoutuseCallToolto use shareduseAsyncOperationhook for state managementuseAsyncOperationhook to extract common async operation patterns with deduplication supportIssues found:
sessionIdwhen deduplication is enabledwidgetsDevServer.tsthat removes unused variable destructuringThe refactoring is well-executed, consolidating duplicate state management logic into a reusable hook. The checkout implementation follows existing patterns from
useCallTool, providing both callback-based and async/await APIs.Confidence Score: 4/5
use-checkout.tsfile could benefit from addressing the sessionId state management if handling rapid sequential checkouts is a requirement