-
Notifications
You must be signed in to change notification settings - Fork 378
[2025-10] Add cartGiftCardCodesAdd mutation and remove Update filtering #3284
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: 2025-10-sfapi-caapi-update
Are you sure you want to change the base?
[2025-10] Add cartGiftCardCodesAdd mutation and remove Update filtering #3284
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
|
This pull request has been marked as stale due to inactivity for 60 days. If no further activity occurs, it will be closed in 7 days. |
Tests verify requirements for new Add mutation: - Add single gift card code - Add multiple codes in one call - Handle empty array - Override cartFragment - Mutation includes userErrors, warnings, @incontext - NO duplicate filtering (thin wrapper pattern) Test Results: FAILING (expected) Error: Failed to resolve import "./cartGiftCardCodesAddDefault" Reason: Module doesn't exist yet (TDD RED phase) Tests include validation that implementation does NOT contain: - unique filtering - filter() calls - indexOf() duplicate checking This follows thin wrapper pattern per investigation-3271.md findings that Add mutations should delegate to API. Related: #3271
Implementation: - Thin wrapper pattern (no duplicate filtering per investigation) - Follows cartGiftCardCodesRemove structure - JSDoc explains append vs replace semantics - Uses giftCardCodes (strings) not IDs Architectural decision: - NO duplicate filtering (API handles case-insensitive normalization) - Consistent with cartLinesAdd, cartDeliveryAddressesAdd patterns - All Add mutations are thin wrappers (0/3 filter) - Documented in investigation-3271.md Test Results: All 7 tests passing ✅ - Basic functionality (single/multiple codes) - Empty array handling - CartFragment override - Mutation structure validation - Duplicate codes pass through (no filtering) TypeScript: ✅ Clean Related: #3271
Tests verify that: - Duplicate codes pass through to API without filtering - Case-insensitive codes handled by API (GIFT123 vs gift123) - Aligns with API 2025-10 behavior (case-insensitive normalization) Related: investigation-3271.md E-016, E-017, E-019
Breaking change for API 2025-10: - Removes client-side unique code filtering - Passes codes directly to Storefront API - API handles case-insensitive normalization - Consistent with Add/Remove mutations (thin wrapper pattern) - Updated JSDoc to document behavior change Evidence: - API schema describes codes as 'case-insensitive' - No DUPLICATE_GIFT_CARD error exists in CartErrorCode/CartWarningCode - Filtering was copy-paste from discount codes (E-016) - All Add/Remove mutations delegate to API without filtering Closes part of #3271
Complete integration of addGiftCardCodes: - Added CartForm action type GiftCardCodesAdd - Exported addGiftCardCodes from createCartHandler - Added doc file with related mutations - Added JS/TS example files - Exported function from package index - Updated all test assertions Tests: ✅ All 447 passing TypeScript: ✅ Clean Related: #3271
Changes: - Added AddGiftCardForm component (uses GiftCardCodesAdd action) - Updated cart.tsx to handle GiftCardCodesAdd action - Changed gift card input form to use Add instead of Update - Keeps UpdateGiftCardForm for backward compatibility Users can now add gift cards without replacing existing ones in skeleton. Related: #3271
481762f to
68315f9
Compare
199a5db to
2652707
Compare
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
Pull request overview
This PR adds support for the Storefront API 2025-10 cartGiftCardCodesAdd mutation, enabling incremental addition of gift card codes without replacing existing ones. It also removes legacy duplicate filtering from cartGiftCardCodesUpdate to align with the thin wrapper architecture pattern used across all other cart mutations.
Changes:
- Added
cartGiftCardCodesAddmutation wrapper with comprehensive tests and documentation - Removed client-side duplicate filtering from
cartGiftCardCodesUpdate - Updated skeleton template to use Add action instead of Update for applying gift cards
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/hydrogen/src/cart/queries/cartGiftCardCodesAddDefault.ts | New mutation implementation following thin wrapper pattern |
| packages/hydrogen/src/cart/queries/cartGiftCardCodesAddDefault.test.ts | Comprehensive test suite with 7 tests covering basic functionality and edge cases |
| packages/hydrogen/src/cart/queries/cartGiftCardCodesAddDefault.doc.ts | Documentation metadata for API reference |
| packages/hydrogen/src/cart/queries/cartGiftCardCodesAddDefault.example.ts/.js | Usage examples in TypeScript and JavaScript |
| packages/hydrogen/src/cart/queries/cartGiftCardCodeUpdateDefault.ts | Removed duplicate filtering logic and updated documentation |
| packages/hydrogen/src/cart/queries/cartGiftCardCodesUpdateDefault.test.ts | Added tests verifying no filtering occurs |
| packages/hydrogen/src/cart/createCartHandler.ts | Integrated addGiftCardCodes method into cart handler |
| packages/hydrogen/src/cart/createCartHandler.test.ts | Updated test expectations for new method count |
| packages/hydrogen/src/cart/CartForm.tsx | Added GiftCardCodesAdd action type and type definitions |
| packages/hydrogen/src/cart/CartForm.test.tsx | Added test for new action constant |
| packages/hydrogen/src/index.ts | Exported new cartGiftCardCodesAddDefault function |
| templates/skeleton/app/routes/cart.tsx | Added GiftCardCodesAdd handler and removed code combining logic from Update handler |
| templates/skeleton/app/components/CartSummary.tsx | Replaced UpdateGiftCardForm with AddGiftCardForm for applying gift cards |
| .changeset/gift-card-add-mutation.md | Comprehensive changeset documenting breaking changes and migration path |
Comments suppressed due to low confidence (1)
templates/skeleton/app/components/CartSummary.tsx:228
- The UpdateGiftCardForm component is now unused after switching to AddGiftCardForm. This dead code should be removed to avoid confusion and reduce maintenance burden.
function UpdateGiftCardForm({
giftCardCodes,
saveAppliedCode,
fetcherKey,
children,
}: {
giftCardCodes?: string[];
saveAppliedCode?: (code: string) => void;
fetcherKey?: string;
children: React.ReactNode;
}) {
return (
<CartForm
fetcherKey={fetcherKey}
route="/cart"
action={CartForm.ACTIONS.GiftCardCodesUpdate}
inputs={{
giftCardCodes: giftCardCodes || [],
}}
>
{(fetcher: FetcherWithComponents<any>) => {
const code = fetcher.formData?.get('giftCardCode');
if (code && saveAppliedCode) {
saveAppliedCode(code as string);
}
return children;
}}
</CartForm>
);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case CartForm.ACTIONS.GiftCardCodesUpdate: { | ||
| const formGiftCardCode = inputs.giftCardCode; | ||
|
|
||
| // User inputted gift card code | ||
| const giftCardCodes = ( | ||
| formGiftCardCode ? [formGiftCardCode] : [] | ||
| ) as string[]; | ||
|
|
||
| // Combine gift card codes already applied on cart | ||
| giftCardCodes.push(...inputs.giftCardCodes); | ||
|
|
||
| result = await cart.updateGiftCardCodes(giftCardCodes); | ||
| break; | ||
| } |
Copilot
AI
Jan 19, 2026
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.
The GiftCardCodesUpdate handler now only uses the single input code without combining it with existing codes. This means calling this action would replace all existing gift cards with just the new code, losing previously applied gift cards. Since the template now uses GiftCardCodesAdd, this handler may never be called, but if it is called (e.g., through custom code), it will exhibit unexpected behavior. Consider either removing this case if it's no longer needed, or documenting that it replaces all codes, or preserving the combining behavior like DiscountCodesUpdate does.
graygilmore
left a 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.
Core functionality of the mutation looks great but I'm a little confused by some of the periphery work.
|
|
||
| describe('cartGiftCardCodesAddDefault', () => { | ||
| describe('basic functionality', () => { | ||
| it('should add gift card codes to cart without replacing existing ones', async () => { |
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.
Is this test actually doing what it says?
| 'WELCOME25', | ||
| ]); | ||
|
|
||
| expect(result.cart).toHaveProperty('id', CART_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.
Similar to this test, I'm not actually sure what it's testing. Is the CART_ID value somehow tied to how all of the gift card codes being added?
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 whole file could maybe use a once over (or maybe I'm not getting something)
| await cart.addGiftCardCodes(['NEW_CODE']); | ||
| ``` | ||
|
|
||
| ## Breaking Change: cartGiftCardCodesUpdate |
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.
I'm waffling on this being considered a breaking change since it's so minimal but I guess it is a change in behaviour so we need to mark it as a break?
| 'Creates a function that adds gift card codes to a cart without replacing existing ones', | ||
| type: 'utility', | ||
| defaultExample: { | ||
| description: 'This is the default example', |
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.
Does this content show up somewhere? If so could we change it to explain what the example is showing?
| expect(result.userErrors?.[0]).toContain(cartFragment); | ||
| }); | ||
|
|
||
| describe('no duplicate filtering (API 2025-10+)', () => { |
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.
Feels a little weird to add tests to something we aren't doing.
| /** | ||
| * Updates (replaces) gift card codes in the cart. | ||
| * | ||
| * This function no longer filters duplicate codes internally. |
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 function no longer filters duplicate codes internally. |
| ); | ||
| } | ||
|
|
||
| function UpdateGiftCardForm({ |
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.
Is UpdateGiftCardForm used anymore? Should we remove it?
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.
For my own learning: why do all of these have Default in the file name?

WHY are these changes introduced?
Fixes #3271
Storefront API 2025-10 added
cartGiftCardCodesAddmutation for appending gift card codes without replacing existing ones. Hydrogen only implementedcartGiftCardCodesRemovein PR #3128, leaving users without a way to add codes incrementally. Since the API only returns the last 4 digits of applied gift cards (security constraint), users cannot fetch existing codes to preserve them when using the Update mutation.This PR also removes legacy duplicate filtering from
cartGiftCardCodesUpdateto align with API 2025-10 thin wrapper architecture.WHAT is this pull request doing?
New Feature: cartGiftCardCodesAdd
Adds thin wrapper for
cartGiftCardCodesAddmutation following the established Add mutation pattern.Files created:
cartGiftCardCodesAddDefault.ts- Core implementation (no duplicate filtering)cartGiftCardCodesAddDefault.test.ts- 7 comprehensive testscartGiftCardCodesAddDefault.doc.ts- Documentation metadatacartGiftCardCodesAddDefault.example.js/ts- Usage examplesIntegration:
createCartHandlerasaddGiftCardCodesCartForm.ACTIONS.GiftCardCodesAddaction typeUsage:
Breaking Change: cartGiftCardCodesUpdate
Removed client-side duplicate code filtering to align with thin wrapper pattern.
Before:
After:
Architecture Decision:
Migration: If you need client-side deduplication:
HOW to test your changes?
🎩 Top Hat
Prerequisites
Testing Steps
Feature 1: Add Gift Card Codes
app/routes/test-gift-cards.tsx):app/routes/api.gift-cards.tsx):Feature 2: Update No Longer Filters
Edge Cases to Test
Validation Checklist
Checklist
Summary of Changes
New:
cart.addGiftCardCodes(codes)- Append codes without replacingCartForm.ACTIONS.GiftCardCodesAdd- Form actionBreaking:
cart.updateGiftCardCodes(codes)- No longer filters duplicates client-sideFiles: 13 files, +400/-9 lines
Tests: 7 new tests, all passing
Architecture: 100% thin wrapper consistency (was 78%)