feat(shop): redesign /shop pages with editorial theme and Tailwind primitives#843
Merged
tannerlinsley merged 1 commit intomainfrom Apr 21, 2026
Merged
feat(shop): redesign /shop pages with editorial theme and Tailwind primitives#843tannerlinsley merged 1 commit intomainfrom
tannerlinsley merged 1 commit intomainfrom
Conversation
…imitives Scoped-only rewrite of the /shop landing, PDP, cart, drawer, and collection/search/policy pages to match a new editorial merch-store direction (cyan accent, DM Sans display, JetBrains Mono for prices/meta). Theme tokens live on a `.shop-scope` wrapper with light and dark variants, exposed to Tailwind via `@theme inline` so authoring stays in utility classes. New primitives under `src/components/shop/ui/` (Button, Badge, Chip, Size, Qty, Select, Input, Tab, Panel, Pulse, Mono, Crumb) and composed pieces (Hero, DropCard, Strip, Specs, Note) back the pages. Global site, navbar, and non-shop routes are unchanged; `shop.css` and the DM Sans / JetBrains Mono fonts only load on /shop. `ProductListItem` gains `productType` and `tags` to power the hover tag + New/Low-stock badges.
✅ Deploy Preview for tanstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR refactors the shop UI system by introducing a design-token-based theming approach, removing the legacy Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/shop.collections.$handle.tsx (1)
88-108:⚠️ Potential issue | 🟠 MajorIgnore stale “load more” responses after the route changes.
Line 104 appends whatever page resolves last. If a user clicks Load more and then changes the collection or sort before that request finishes, the old page can be merged into the new result set. Carry the request's
handle/sortId/cursorthroughmutate()and droponSuccessresults that no longer match current route state.One way to guard the response
- const loadMore = useMutation({ - mutationFn: async () => { - if (!endCursor) return null - const sortOption = resolveCollectionSortOption(sortId) + const loadMore = useMutation({ + mutationFn: async ({ + handle, + cursor, + sortId, + }: { + handle: string + cursor: string + sortId: ValidCollectionSortId + }) => { + const sortOption = resolveCollectionSortOption(sortId) return getCollection({ data: { - handle: collection.handle, + handle, first: PAGE_SIZE, - after: endCursor, + after: cursor, sortKey: sortOption.key, reverse: sortOption.reverse, }, }) }, - onSuccess: (next) => { - if (!next) return + onSuccess: (next, vars) => { + if ( + !next || + vars.handle !== collection.handle || + vars.sortId !== sortId + ) { + return + } setAccumulated((prev) => [...prev, ...next.products.nodes]) setEndCursor(next.products.pageInfo.endCursor) setHasNextPage(next.products.pageInfo.hasNextPage) }, })- <ShopButton - onClick={() => loadMore.mutate()} + <ShopButton + onClick={() => { + if (!endCursor) return + loadMore.mutate({ + handle: collection.handle, + cursor: endCursor, + sortId, + }) + }} disabled={loadMore.isPending} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/shop.collections`.$handle.tsx around lines 88 - 108, The loadMore mutation can append stale pages when the route changes; modify the mutate() call and the mutation handlers so the request carries its request-specific params (handle, sortId, cursor) and onSuccess ignores results whose handle/sortId/cursor do not match the current route state. Concretely: include the current collection.handle, sortId, and endCursor in the mutate() input so mutationFn uses those values (rather than outer scopes), and in onSuccess compare the response’s handle/sortId/cursor with the current collection.handle, sortId, and endCursor before calling setAccumulated, setEndCursor, or setHasNextPage (if they differ, drop the response). Ensure mutationFn and onSuccess in loadMore reference these passed-in identifiers so stale responses are rejected.src/routes/shop.products.$handle.tsx (1)
215-238:⚠️ Potential issue | 🟡 MinorAria label may be misleading when more than 5 images exist.
The thumbnail grid is limited to
images.slice(0, 5), but the aria-label on line 221 referencesimages.length, which could be greater than 5. This could confuse screen reader users.🔧 Proposed fix
- {images.slice(0, 5).map((img, i) => ( + {images.slice(0, 5).map((img, i) => { + const displayedCount = Math.min(images.length, 5) + return ( <button key={`${img.url}-${i}`} type="button" onClick={() => onChange(i)} - aria-label={`View image ${i + 1} of ${images.length}`} + aria-label={`View image ${i + 1} of ${displayedCount}`} aria-current={i === activeIndex}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/shop.products`.$handle.tsx around lines 215 - 238, The aria-label on the thumbnail buttons incorrectly reports images.length while the UI only renders images.slice(0, 5); update the label to reflect the actual number of thumbnails shown (e.g., use the sliced array length or Math.min(images.length, 5)) in the map that renders the buttons (the block using images.slice(0, 5).map, onChange, activeIndex, and ProductImage) so screen readers see the correct "View image X of Y" count; keep aria-current logic unchanged.
🧹 Nitpick comments (6)
src/components/shop/ui/Crumb.tsx (1)
12-43: Prefer list semantics for breadcrumb structure.This works, but using
<ol>/<li>inside the<nav>gives better breadcrumb semantics for assistive tech than plain spans at Line [12]–Line [43].♻️ Suggested semantic structure
- <nav - aria-label="Breadcrumb" - className="font-shop-mono text-[11px] tracking-[0.12em] uppercase text-shop-muted flex items-center gap-2 flex-wrap" - > - {crumbs.map((crumb, i) => { + <nav aria-label="Breadcrumb"> + <ol className="font-shop-mono text-[11px] tracking-[0.12em] uppercase text-shop-muted flex items-center gap-2 flex-wrap"> + {crumbs.map((crumb, i) => { const isLast = i === crumbs.length - 1 return ( - <React.Fragment key={`${crumb.label}-${i}`}> + <li key={`${crumb.label}-${i}`} className="inline-flex items-center gap-2"> {crumb.href && !isLast ? ( <Link to={crumb.href} className="text-shop-muted hover:text-shop-text transition-colors" > {crumb.label} </Link> ) : ( <span aria-current={isLast ? 'page' : undefined} className={isLast ? 'text-shop-text font-medium' : undefined} > {crumb.label} </span> )} {isLast ? null : ( <span aria-hidden="true" className="opacity-40"> / </span> )} - </React.Fragment> + </li> ) })} - </nav> + </ol> + </nav>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/shop/ui/Crumb.tsx` around lines 12 - 43, Replace the current nav children (crumbs.map using React.Fragment, spans and separators) with a semantic ordered list: render an <ol> inside the existing <nav>, map crumbs to <li> elements (use the same key logic currently on React.Fragment), place the <Link> or <span> inside each <li> as before, and apply aria-current="page" to the current crumb element (the same isLast check) while keeping the visual separator (e.g., "/" ) but ensure it is not inside the <li> for the current item or mark it aria-hidden; update the crumbs.map usage and remove React.Fragment so each breadcrumb is an <li> within the new <ol>.src/routes/shop.products.$handle.tsx (2)
400-416: Acknowledge XSS consideration for Shopify HTML content.The
dangerouslySetInnerHTMLusage is flagged by static analysis. While Shopify sanitizesdescriptionHtmlserver-side, adding a brief comment acknowledging this trust assumption would document the security decision.📝 Suggested documentation
function ProductDescription({ html }: { html: string }) { return ( <div className="..." - // eslint-disable-next-line react/no-danger + // Shopify sanitizes descriptionHtml; trusted source. + // eslint-disable-next-line react/no-danger dangerouslySetInnerHTML={{ __html: html }} /> ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/shop.products`.$handle.tsx around lines 400 - 416, Add a short comment inside the ProductDescription component next to the dangerouslySetInnerHTML usage that explicitly states you are relying on Shopify to sanitize descriptionHtml server-side (e.g., "descriptionHtml is sanitized by Shopify; trusted input used here") and note that any future changes should re-validate or re-sanitize input before rendering; reference the ProductDescription function and the html prop in the comment so reviewers know this is an intentional security decision.
354-359: Redundant clamping in nested onChange handlers.The
onChangecallback on line 357 clamps toMath.max(1, n), but the parentQuantityAddalready receives a clampedonChangefrom line 139 ((n) => setQuantity(Math.max(1, n))). Additionally,ShopQtyhasmin={1}which disables the minus button at quantity 1. One layer of clamping would suffice.♻️ Simplified version
<ShopQty quantity={quantity} - onChange={(n) => onChange(Math.max(1, n))} + onChange={onChange} min={1} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/shop.products`.$handle.tsx around lines 354 - 359, The inner clamping is redundant: remove the Math.max(1, n) in the nested onChange passed to the ShopQty instance and forward the value directly (e.g., change onChange={(n) => onChange(Math.max(1, n))} to onChange={onChange} or onChange={(n) => onChange(n)}), since the parent QuantityAdd/setQuantity already applies Math.max(1, n) and ShopQty also has min={1}; keep the parent clamp and ShopQty's min but eliminate the duplicate clamp in the nested handler.src/components/shop/CartDrawer.tsx (1)
193-229: Consider extracting toShopQtyfor consistency.The cart drawer uses an inline quantity stepper while
shop.cart.tsxuses the sharedShopQtycomponent. While the compact drawer layout may justify the inline implementation, usingShopQtywithsize="sm"would ensure consistent behavior and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/shop/CartDrawer.tsx` around lines 193 - 229, Replace the inline quantity stepper in CartDrawer with the shared ShopQty component to ensure consistency and reduce duplication: locate the div containing the two buttons and span that reference line.quantity, update.mutate, remove.mutate, and isBusy, and swap it out for <ShopQty size="sm" value={line.quantity} onDecrease={() => { if (line.quantity <= 1) remove.mutate({ lineId: line.id }) else update.mutate({ lineId: line.id, quantity: line.quantity - 1 }) }} onIncrease={() => update.mutate({ lineId: line.id, quantity: line.quantity + 1 })} disabled={isBusy} /> (use ShopQty props/handlers matching your shared component API) so behavior remains identical while using the shared component.src/routes/shop.cart.tsx (1)
115-120: Consider extractingisSuccessandresetto avoid effect re-runs.The
applymutation object is a new reference on each render, which means this effect's dependency array changes every render. While React Query's mutation objects are stable in practice, extracting the specific values would make the dependencies clearer and prevent potential issues.♻️ Suggested refactor
+ const { isSuccess, reset } = apply + React.useEffect(() => { - if (apply.isSuccess) { + if (isSuccess) { setInput('') - apply.reset() + reset() } - }, [apply.isSuccess, apply]) + }, [isSuccess, reset])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/shop.cart.tsx` around lines 115 - 120, The effect currently depends on the whole `apply` mutation object which can change identity; extract `const isSuccess = apply.isSuccess` and `const reset = apply.reset` (or destructure `{ isSuccess, reset } = apply`) at the top of the component and update the effect to use `isSuccess` and `reset` in its body and dependency array (call `reset()` and `setInput('')` when `isSuccess` is true) so the effect only re-runs when those specific values change instead of the entire `apply` object.src/components/shop/ShopDropCard.tsx (1)
44-57: Consider explicit route typing for thecta.hrefprop.The current implementation uses
Linkwithto={cta.href}wherehrefis typed as an arbitrary string. While the single current usage with/shopworks correctly, TanStack Router'stoprop expects internal routes and will produce TypeScript errors if external URLs are passed. Ifcta.hrefis intended only for internal routes, use a route-specific type. If external URLs should be supported in the future, consider conditionally rendering a plain<a>element for external links.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/shop/ShopDropCard.tsx` around lines 44 - 57, The cta.href is typed as a plain string but is passed to the TanStack Router Link via to={cta.href}; update the cta prop type to use the router's internal route type (e.g., To or RouteUrl type from TanStack Router) or make it a discriminated union like { href: To; external?: false } | { href: string; external: true } and then conditionally render: use <Link to={cta.href} /> for internal routes and a plain <a href={cta.href} target="_blank" rel="noopener noreferrer"> for external URLs; adjust the ShopDropCard props/interface and the usage of cta.href accordingly to eliminate TypeScript errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/shop/ProductCard.tsx`:
- Around line 29-34: The fallback label logic for tag (const tag =
product.productType || product.tags?.[0] || '') can accidentally use status
badges like 'new' or 'low-stock'; update the selection to filter out those
status tags using the existing isNew and isLowStock checks (or a combined
predicate) before picking product.tags[0]. In practice, derive a cleanedTags
array = product.tags?.filter(t => !isNew && !isLowStock for that t) or implement
a helper (e.g., isStatusTag) and use product.productType || cleanedTags?.[0] ||
'' so status badges are excluded from the category chip and mono caption.
In `@src/components/shop/ShopNote.tsx`:
- Line 20: The ShopNote component currently renders children inside an inline
<span>, which breaks layout when children are block elements; update the render
in ShopNote (the element that currently reads "<span>{children}</span>") to use
a block-level container (e.g., "<div>{children}</div>") or otherwise allow
children to render as-is (React.Fragment) while preserving any existing
props/styles/className defined on ShopNote.
In `@src/components/shop/ShopSpecs.tsx`:
- Around line 7-10: The list rendering in ShopSpecs uses specs.map and sets the
React key to spec.label which can collide when labels repeat; update the key to
a stable unique identifier such as an id or the array index combined with label
(e.g., use spec.id if available, otherwise use `${spec.label}-${i}`) in the JSX
key prop inside the map to ensure stable reconciliation for the ShopSpecs
component.
In `@src/components/shop/ShopStrip.tsx`:
- Around line 7-9: The map in ShopStrip.tsx uses items.map((item) => ...) and
sets key={item}, which can create duplicate React keys for identical labels;
change the key to a stable unique value such as a provided id (e.g., item.id) or
combine the label with the index (e.g., `${item}-${index}`) in the map callback
so each rendered element has a unique key, and update the map signature to
include the index parameter if needed.
In `@src/components/shop/ui/Button.tsx`:
- Around line 30-33: The JSX <button> in the Button component currently lacks an
explicit type so it can accidentally submit parent forms; update the button
element in the Button component to set a safe default type by ensuring the
rendered props include type={props.type ?? "button"} (or similar) so callers can
override but the default is "button"; locate the button element that spreads
{...rest} and add this defaulting logic around the type prop.
In `@src/components/shop/ui/Select.tsx`:
- Around line 17-18: In Select.tsx the inline SVG data-URI hardcodes
stroke='%23a8a8b0' which prevents the chevron from following the shop-muted
theme; update the Select component to render the chevron using the current theme
color instead of a fixed hex—either replace the data-URI with an SVG that uses
stroke='currentColor' (and apply the existing text/shop-muted token class to the
container) or render the SVG as a separate JSX element positioned like the
background image and style it via the shop-muted CSS variable; locate the class
string that contains bg-[url("data:image/svg+xml...")] in the Select component
and change it accordingly so the chevron inherits the theme.
In `@src/routes/shop.index.tsx`:
- Around line 197-221: The current ternary hides pagination when products.length
=== 0; update rendering so the empty-state message (products.length === 0) does
not short-circuit the pagination block—render the "No products yet" div when
products is empty but always allow the hasNextPage block with ShopButton and
loadMore to render if hasNextPage is true (i.e., move or duplicate the
hasNextPage check out of the products.length === 0 branch); locate references to
products, hasNextPage, loadMore, ProductCard, and ShopButton in this component
and ensure the pagination UI is rendered independently of the empty-list
message.
In `@src/routes/shop.policies`.$handle.tsx:
- Around line 46-48: The JSX injects policy.body via dangerouslySetInnerHTML
without sanitization; fix this by importing and using an HTML sanitizer (e.g.,
DOMPurify) to produce a sanitized string (e.g., sanitizedBody) before passing it
to dangerouslySetInnerHTML; update the component that renders policy (reference
the policy variable and the JSX element using dangerouslySetInnerHTML) to use
sanitizedBody instead of policy.body, and ensure DOMPurify is set up
appropriately for your runtime (install/import dompurify and types, or use
server-safe setup if rendering on the server).
In `@src/routes/shop.search.tsx`:
- Around line 92-100: The search input lacks an accessible label; update the
ShopInput usage to include an accessible label by either adding a visible
<label> with htmlFor matching an id prop on ShopInput (e.g., give ShopInput
id="shop-search" and add a visually-hidden label text "Search products") or at
minimum add an aria-label="Search products" prop to ShopInput so screen readers
can announce the field; ensure the change is applied where SearchIcon,
ShopInput, inputValue and setInputValue are used so the input retains its
id/aria-label and behavior.
---
Outside diff comments:
In `@src/routes/shop.collections`.$handle.tsx:
- Around line 88-108: The loadMore mutation can append stale pages when the
route changes; modify the mutate() call and the mutation handlers so the request
carries its request-specific params (handle, sortId, cursor) and onSuccess
ignores results whose handle/sortId/cursor do not match the current route state.
Concretely: include the current collection.handle, sortId, and endCursor in the
mutate() input so mutationFn uses those values (rather than outer scopes), and
in onSuccess compare the response’s handle/sortId/cursor with the current
collection.handle, sortId, and endCursor before calling setAccumulated,
setEndCursor, or setHasNextPage (if they differ, drop the response). Ensure
mutationFn and onSuccess in loadMore reference these passed-in identifiers so
stale responses are rejected.
In `@src/routes/shop.products`.$handle.tsx:
- Around line 215-238: The aria-label on the thumbnail buttons incorrectly
reports images.length while the UI only renders images.slice(0, 5); update the
label to reflect the actual number of thumbnails shown (e.g., use the sliced
array length or Math.min(images.length, 5)) in the map that renders the buttons
(the block using images.slice(0, 5).map, onChange, activeIndex, and
ProductImage) so screen readers see the correct "View image X of Y" count; keep
aria-current logic unchanged.
---
Nitpick comments:
In `@src/components/shop/CartDrawer.tsx`:
- Around line 193-229: Replace the inline quantity stepper in CartDrawer with
the shared ShopQty component to ensure consistency and reduce duplication:
locate the div containing the two buttons and span that reference line.quantity,
update.mutate, remove.mutate, and isBusy, and swap it out for <ShopQty size="sm"
value={line.quantity} onDecrease={() => { if (line.quantity <= 1)
remove.mutate({ lineId: line.id }) else update.mutate({ lineId: line.id,
quantity: line.quantity - 1 }) }} onIncrease={() => update.mutate({ lineId:
line.id, quantity: line.quantity + 1 })} disabled={isBusy} /> (use ShopQty
props/handlers matching your shared component API) so behavior remains identical
while using the shared component.
In `@src/components/shop/ShopDropCard.tsx`:
- Around line 44-57: The cta.href is typed as a plain string but is passed to
the TanStack Router Link via to={cta.href}; update the cta prop type to use the
router's internal route type (e.g., To or RouteUrl type from TanStack Router) or
make it a discriminated union like { href: To; external?: false } | { href:
string; external: true } and then conditionally render: use <Link to={cta.href}
/> for internal routes and a plain <a href={cta.href} target="_blank"
rel="noopener noreferrer"> for external URLs; adjust the ShopDropCard
props/interface and the usage of cta.href accordingly to eliminate TypeScript
errors.
In `@src/components/shop/ui/Crumb.tsx`:
- Around line 12-43: Replace the current nav children (crumbs.map using
React.Fragment, spans and separators) with a semantic ordered list: render an
<ol> inside the existing <nav>, map crumbs to <li> elements (use the same key
logic currently on React.Fragment), place the <Link> or <span> inside each <li>
as before, and apply aria-current="page" to the current crumb element (the same
isLast check) while keeping the visual separator (e.g., "/" ) but ensure it is
not inside the <li> for the current item or mark it aria-hidden; update the
crumbs.map usage and remove React.Fragment so each breadcrumb is an <li> within
the new <ol>.
In `@src/routes/shop.cart.tsx`:
- Around line 115-120: The effect currently depends on the whole `apply`
mutation object which can change identity; extract `const isSuccess =
apply.isSuccess` and `const reset = apply.reset` (or destructure `{ isSuccess,
reset } = apply`) at the top of the component and update the effect to use
`isSuccess` and `reset` in its body and dependency array (call `reset()` and
`setInput('')` when `isSuccess` is true) so the effect only re-runs when those
specific values change instead of the entire `apply` object.
In `@src/routes/shop.products`.$handle.tsx:
- Around line 400-416: Add a short comment inside the ProductDescription
component next to the dangerouslySetInnerHTML usage that explicitly states you
are relying on Shopify to sanitize descriptionHtml server-side (e.g.,
"descriptionHtml is sanitized by Shopify; trusted input used here") and note
that any future changes should re-validate or re-sanitize input before
rendering; reference the ProductDescription function and the html prop in the
comment so reviewers know this is an intentional security decision.
- Around line 354-359: The inner clamping is redundant: remove the Math.max(1,
n) in the nested onChange passed to the ShopQty instance and forward the value
directly (e.g., change onChange={(n) => onChange(Math.max(1, n))} to
onChange={onChange} or onChange={(n) => onChange(n)}), since the parent
QuantityAdd/setQuantity already applies Math.max(1, n) and ShopQty also has
min={1}; keep the parent clamp and ShopQty's min but eliminate the duplicate
clamp in the nested handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cc9dc6f-85f2-4293-84c9-791d8e50ce06
📒 Files selected for processing (32)
src/components/shop/Breadcrumbs.tsxsrc/components/shop/CartDrawer.tsxsrc/components/shop/ProductCard.tsxsrc/components/shop/ShopDropCard.tsxsrc/components/shop/ShopHero.tsxsrc/components/shop/ShopLayout.tsxsrc/components/shop/ShopNote.tsxsrc/components/shop/ShopSpecs.tsxsrc/components/shop/ShopStrip.tsxsrc/components/shop/ui/Badge.tsxsrc/components/shop/ui/Button.tsxsrc/components/shop/ui/Chip.tsxsrc/components/shop/ui/Crumb.tsxsrc/components/shop/ui/Input.tsxsrc/components/shop/ui/Mono.tsxsrc/components/shop/ui/Panel.tsxsrc/components/shop/ui/Pulse.tsxsrc/components/shop/ui/Qty.tsxsrc/components/shop/ui/Select.tsxsrc/components/shop/ui/Size.tsxsrc/components/shop/ui/Tab.tsxsrc/components/shop/ui/index.tssrc/routes/shop.cart.tsxsrc/routes/shop.collections.$handle.tsxsrc/routes/shop.index.tsxsrc/routes/shop.pages.$handle.tsxsrc/routes/shop.policies.$handle.tsxsrc/routes/shop.products.$handle.tsxsrc/routes/shop.search.tsxsrc/routes/shop.tsxsrc/styles/shop.csssrc/utils/shopify-queries.ts
💤 Files with no reviewable changes (1)
- src/components/shop/Breadcrumbs.tsx
Comment on lines
+29
to
+34
| const isNew = product.tags?.some((t) => t.toLowerCase() === 'new') | ||
| const isLowStock = product.tags?.some( | ||
| (t) => t.toLowerCase() === 'low-stock' || t.toLowerCase() === 'low stock', | ||
| ) | ||
|
|
||
| const tag = product.productType || product.tags?.[0] || '' |
There was a problem hiding this comment.
Filter status tags out of the fallback display label.
Line 34 falls back to product.tags?.[0], so products tagged new or low-stock can show those internal status tags again in the category chip and mono caption. Exclude badge/status tags before choosing a display label.
Example fix
+ const statusTags = new Set(['new', 'sale', 'low-stock', 'low stock'])
+
const isNew = product.tags?.some((t) => t.toLowerCase() === 'new')
const isLowStock = product.tags?.some(
(t) => t.toLowerCase() === 'low-stock' || t.toLowerCase() === 'low stock',
)
- const tag = product.productType || product.tags?.[0] || ''
+ const tag =
+ product.productType ||
+ product.tags?.find((t) => !statusTags.has(t.toLowerCase())) ||
+ ''📝 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.
Suggested change
| const isNew = product.tags?.some((t) => t.toLowerCase() === 'new') | |
| const isLowStock = product.tags?.some( | |
| (t) => t.toLowerCase() === 'low-stock' || t.toLowerCase() === 'low stock', | |
| ) | |
| const tag = product.productType || product.tags?.[0] || '' | |
| const statusTags = new Set(['new', 'sale', 'low-stock', 'low stock']) | |
| const isNew = product.tags?.some((t) => t.toLowerCase() === 'new') | |
| const isLowStock = product.tags?.some( | |
| (t) => t.toLowerCase() === 'low-stock' || t.toLowerCase() === 'low stock', | |
| ) | |
| const tag = | |
| product.productType || | |
| product.tags?.find((t) => !statusTags.has(t.toLowerCase())) || | |
| '' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/shop/ProductCard.tsx` around lines 29 - 34, The fallback label
logic for tag (const tag = product.productType || product.tags?.[0] || '') can
accidentally use status badges like 'new' or 'low-stock'; update the selection
to filter out those status tags using the existing isNew and isLowStock checks
(or a combined predicate) before picking product.tags[0]. In practice, derive a
cleanedTags array = product.tags?.filter(t => !isNew && !isLowStock for that t)
or implement a helper (e.g., isStatusTag) and use product.productType ||
cleanedTags?.[0] || '' so status badges are excluded from the category chip and
mono caption.
| {pill} | ||
| </span> | ||
| ) : null} | ||
| <span>{children}</span> |
There was a problem hiding this comment.
Render children in a block container instead of <span>.
children accepts any React.ReactNode; wrapping it in <span> can break semantics/layout when a block element is passed.
Proposed fix
- <span>{children}</span>
+ <div>{children}</div>📝 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.
Suggested change
| <span>{children}</span> | |
| <div>{children}</div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/shop/ShopNote.tsx` at line 20, The ShopNote component
currently renders children inside an inline <span>, which breaks layout when
children are block elements; update the render in ShopNote (the element that
currently reads "<span>{children}</span>") to use a block-level container (e.g.,
"<div>{children}</div>") or otherwise allow children to render as-is
(React.Fragment) while preserving any existing props/styles/className defined on
ShopNote.
Comment on lines
+7
to
+10
| {specs.map((spec, i) => ( | ||
| <div | ||
| key={spec.label} | ||
| className={ |
There was a problem hiding this comment.
Keying by spec.label can collide for repeated labels.
If two specs share the same label, React reconciliation becomes unstable.
Proposed fix
- {specs.map((spec, i) => (
+ {specs.map((spec, i) => (
<div
- key={spec.label}
+ key={`${spec.label}-${i}`}📝 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.
Suggested change
| {specs.map((spec, i) => ( | |
| <div | |
| key={spec.label} | |
| className={ | |
| {specs.map((spec, i) => ( | |
| <div | |
| key={`${spec.label}-${i}`} | |
| className={ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/shop/ShopSpecs.tsx` around lines 7 - 10, The list rendering in
ShopSpecs uses specs.map and sets the React key to spec.label which can collide
when labels repeat; update the key to a stable unique identifier such as an id
or the array index combined with label (e.g., use spec.id if available,
otherwise use `${spec.label}-${i}`) in the JSX key prop inside the map to ensure
stable reconciliation for the ShopSpecs component.
Comment on lines
+7
to
+9
| {items.map((item) => ( | ||
| <span key={item} className="inline-flex items-center gap-2"> | ||
| <span |
There was a problem hiding this comment.
Avoid non-unique React keys in the strip items.
Using only item as key can collide when duplicate labels appear.
Proposed fix
- {items.map((item) => (
- <span key={item} className="inline-flex items-center gap-2">
+ {items.map((item, i) => (
+ <span key={`${item}-${i}`} className="inline-flex items-center gap-2">📝 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.
Suggested change
| {items.map((item) => ( | |
| <span key={item} className="inline-flex items-center gap-2"> | |
| <span | |
| {items.map((item, i) => ( | |
| <span key={`${item}-${i}`} className="inline-flex items-center gap-2"> | |
| <span |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/shop/ShopStrip.tsx` around lines 7 - 9, The map in
ShopStrip.tsx uses items.map((item) => ...) and sets key={item}, which can
create duplicate React keys for identical labels; change the key to a stable
unique value such as a provided id (e.g., item.id) or combine the label with the
index (e.g., `${item}-${index}`) in the map callback so each rendered element
has a unique key, and update the map signature to include the index parameter if
needed.
Comment on lines
+30
to
+33
| <button | ||
| ref={ref} | ||
| {...rest} | ||
| className={twMerge( |
There was a problem hiding this comment.
Set a safe default button type to prevent accidental form submits.
Without an explicit type, this button submits parent forms by default.
Proposed fix
export const ShopButton = React.forwardRef<HTMLButtonElement, Props>(
function ShopButton(
- { variant = 'outline', fullWidth, className, children, ...rest },
+ { variant = 'outline', fullWidth, className, children, type = 'button', ...rest },
ref,
) {
return (
<button
ref={ref}
+ type={type}
{...rest}
className={twMerge(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/shop/ui/Button.tsx` around lines 30 - 33, The JSX <button> in
the Button component currently lacks an explicit type so it can accidentally
submit parent forms; update the button element in the Button component to set a
safe default type by ensuring the rendered props include type={props.type ??
"button"} (or similar) so callers can override but the default is "button";
locate the button element that spreads {...rest} and add this defaulting logic
around the type prop.
Comment on lines
+17
to
+18
| // inline SVG chevron, colored with the muted text token | ||
| "bg-[url(\"data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' viewBox='0 0 10 10'><path d='M2 4l3 3 3-3' stroke='%23a8a8b0' fill='none' stroke-width='1.4'/></svg>\")]", |
There was a problem hiding this comment.
Make the chevron follow the active theme.
Line 18 hardcodes #a8a8b0 inside the SVG data URI, so the arrow no longer tracks the light/dark shop-muted token values from shop.css. In light mode this will stay on the dark-theme gray instead of matching the rest of the control. Render the chevron separately or otherwise derive it from the current theme color instead of baking in a hex value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/shop/ui/Select.tsx` around lines 17 - 18, In Select.tsx the
inline SVG data-URI hardcodes stroke='%23a8a8b0' which prevents the chevron from
following the shop-muted theme; update the Select component to render the
chevron using the current theme color instead of a fixed hex—either replace the
data-URI with an SVG that uses stroke='currentColor' (and apply the existing
text/shop-muted token class to the container) or render the SVG as a separate
JSX element positioned like the background image and style it via the shop-muted
CSS variable; locate the class string that contains
bg-[url("data:image/svg+xml...")] in the Select component and change it
accordingly so the chevron inherits the theme.
Comment on lines
+197
to
+221
| {products.length === 0 ? ( | ||
| <div className="text-center py-24 text-shop-muted"> | ||
| No products yet. Check back soon! | ||
| </div> | ||
| ) : ( | ||
| <> | ||
| <section className="grid gap-x-4 gap-y-5.5 grid-cols-1 xs:grid-cols-2 md:grid-cols-3 lg:grid-cols-4"> | ||
| {products.map((product, i) => ( | ||
| <ProductCard | ||
| key={product.id} | ||
| product={product} | ||
| loading={i < 8 ? 'eager' : 'lazy'} | ||
| /> | ||
| ))} | ||
| </section> | ||
| {hasNextPage ? ( | ||
| <div className="flex justify-center py-8"> | ||
| <ShopButton | ||
| onClick={() => loadMore.mutate()} | ||
| disabled={loadMore.isPending} | ||
| > | ||
| {loadMore.isPending ? 'Loading…' : 'Load more'} | ||
| </ShopButton> | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
Don’t hide pagination when filtered list is temporarily empty.
At Line [197], products.length === 0 short-circuits to an empty state and suppresses “Load more,” even when hasNextPage is true. This can happen after type+sort changes and prevents finding matches on later pages.
✅ Minimal fix
- {products.length === 0 ? (
+ {products.length === 0 && !hasNextPage ? (
<div className="text-center py-24 text-shop-muted">
No products yet. Check back soon!
</div>
) : (
<>
<section className="grid gap-x-4 gap-y-5.5 grid-cols-1 xs:grid-cols-2 md:grid-cols-3 lg:grid-cols-4">
{products.map((product, i) => (
<ProductCard
key={product.id}
product={product}
loading={i < 8 ? 'eager' : 'lazy'}
/>
))}
</section>
{hasNextPage ? (
<div className="flex justify-center py-8">
<ShopButton
onClick={() => loadMore.mutate()}
disabled={loadMore.isPending}
>
{loadMore.isPending ? 'Loading…' : 'Load more'}
</ShopButton>
</div>
) : null}
</>
)}📝 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.
Suggested change
| {products.length === 0 ? ( | |
| <div className="text-center py-24 text-shop-muted"> | |
| No products yet. Check back soon! | |
| </div> | |
| ) : ( | |
| <> | |
| <section className="grid gap-x-4 gap-y-5.5 grid-cols-1 xs:grid-cols-2 md:grid-cols-3 lg:grid-cols-4"> | |
| {products.map((product, i) => ( | |
| <ProductCard | |
| key={product.id} | |
| product={product} | |
| loading={i < 8 ? 'eager' : 'lazy'} | |
| /> | |
| ))} | |
| </section> | |
| {hasNextPage ? ( | |
| <div className="flex justify-center py-8"> | |
| <ShopButton | |
| onClick={() => loadMore.mutate()} | |
| disabled={loadMore.isPending} | |
| > | |
| {loadMore.isPending ? 'Loading…' : 'Load more'} | |
| </ShopButton> | |
| </div> | |
| ) : null} | |
| {products.length === 0 && !hasNextPage ? ( | |
| <div className="text-center py-24 text-shop-muted"> | |
| No products yet. Check back soon! | |
| </div> | |
| ) : ( | |
| <> | |
| <section className="grid gap-x-4 gap-y-5.5 grid-cols-1 xs:grid-cols-2 md:grid-cols-3 lg:grid-cols-4"> | |
| {products.map((product, i) => ( | |
| <ProductCard | |
| key={product.id} | |
| product={product} | |
| loading={i < 8 ? 'eager' : 'lazy'} | |
| /> | |
| ))} | |
| </section> | |
| {hasNextPage ? ( | |
| <div className="flex justify-center py-8"> | |
| <ShopButton | |
| onClick={() => loadMore.mutate()} | |
| disabled={loadMore.isPending} | |
| > | |
| {loadMore.isPending ? 'Loading…' : 'Load more'} | |
| </ShopButton> | |
| </div> | |
| ) : null} | |
| </> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/shop.index.tsx` around lines 197 - 221, The current ternary hides
pagination when products.length === 0; update rendering so the empty-state
message (products.length === 0) does not short-circuit the pagination
block—render the "No products yet" div when products is empty but always allow
the hasNextPage block with ShopButton and loadMore to render if hasNextPage is
true (i.e., move or duplicate the hasNextPage check out of the products.length
=== 0 branch); locate references to products, hasNextPage, loadMore,
ProductCard, and ShopButton in this component and ensure the pagination UI is
rendered independently of the empty-list message.
Comment on lines
46
to
48
| // eslint-disable-next-line react/no-danger | ||
| dangerouslySetInnerHTML={{ __html: policy.body }} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify trust/sanitization path for injected shop HTML.
# Expected: a clear sanitization step before render, or documented trusted source guarantees.
rg -n -C3 "dangerouslySetInnerHTML|policy\.body|page\.body|getShopPolicy|getPage|sanitize|DOMPurify|sanitize-html"Repository: TanStack/tanstack.com
Length of output: 21596
🏁 Script executed:
sed -n '231,280p' src/utils/shop.functions.tsRepository: TanStack/tanstack.com
Length of output: 1631
🏁 Script executed:
rg -n "SHOP_POLICIES_QUERY|type ShopPolicy" src/utils/shop.functions.ts -A 15Repository: TanStack/tanstack.com
Length of output: 1638
🏁 Script executed:
rg -n "SHOP_POLICIES_QUERY" src/utils/shopify-queries.ts -A 30Repository: TanStack/tanstack.com
Length of output: 650
🏁 Script executed:
rg -n "shop\..*Policy|policy\.body|page\.body" src/routes/shop.policies.$handle.tsx src/routes/shop.pages.$handle.tsx -B 5 -A 5Repository: TanStack/tanstack.com
Length of output: 201
🏁 Script executed:
rg -n "policy\.body|page\.body" src/routes/ -B 5 -A 5Repository: TanStack/tanstack.com
Length of output: 3571
🏁 Script executed:
rg -n "Shopify|sanitize|XSS|security" src/utils/shopify-queries.ts src/utils/shopify-api.ts -i -B 2 -A 2Repository: TanStack/tanstack.com
Length of output: 2429
🏁 Script executed:
find src -name "*shopify*" -type fRepository: TanStack/tanstack.com
Length of output: 121
🏁 Script executed:
cat -n src/utils/shopify-format.tsRepository: TanStack/tanstack.com
Length of output: 1431
🏁 Script executed:
rg -n "shopifyServerFetch" src/ -B 5 -A 10 | head -100Repository: TanStack/tanstack.com
Length of output: 5536
🏁 Script executed:
find src -name "fetch.ts" -o -name "fetch.js" | xargs cat -n 2>/dev/nullRepository: TanStack/tanstack.com
Length of output: 3321
🏁 Script executed:
rg -n "policy|storefront" src/utils/shopify-queries.ts -B 3 -A 3 | grep -A 5 -B 5 "policy\|body"Repository: TanStack/tanstack.com
Length of output: 362
🏁 Script executed:
rg -n "\.body" src/utils/shopify-queries.ts -B 5 -A 5Repository: TanStack/tanstack.com
Length of output: 47
🏁 Script executed:
sed -n '600,680p' src/utils/shopify-queries.tsRepository: TanStack/tanstack.com
Length of output: 1806
Sanitize policy.body before injecting HTML.
At line 47, policy.body from the Shopify Storefront API is injected via dangerouslySetInnerHTML without sanitization. Although Shopify is a trusted third-party service, the policy content field can be modified by store administrators. Implement HTML sanitization using a library like DOMPurify to prevent potential XSS if the content becomes compromised or if admin accounts are breached.
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 46-46: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/shop.policies`.$handle.tsx around lines 46 - 48, The JSX injects
policy.body via dangerouslySetInnerHTML without sanitization; fix this by
importing and using an HTML sanitizer (e.g., DOMPurify) to produce a sanitized
string (e.g., sanitizedBody) before passing it to dangerouslySetInnerHTML;
update the component that renders policy (reference the policy variable and the
JSX element using dangerouslySetInnerHTML) to use sanitizedBody instead of
policy.body, and ensure DOMPurify is set up appropriately for your runtime
(install/import dompurify and types, or use server-safe setup if rendering on
the server).
Comment on lines
92
to
100
| <div className="relative flex-1"> | ||
| <SearchIcon className="absolute left-3 top-1/2 -translate-y-1/2 w-4 h-4 text-gray-400" /> | ||
| <input | ||
| <SearchIcon className="absolute left-3 top-1/2 -translate-y-1/2 w-4 h-4 text-shop-muted" /> | ||
| <ShopInput | ||
| type="search" | ||
| value={inputValue} | ||
| onChange={(e) => setInputValue(e.target.value)} | ||
| placeholder="Search products…" | ||
| className="w-full pl-9 pr-3 py-2 rounded-lg border border-gray-300 dark:border-gray-700 bg-white dark:bg-gray-950 text-sm" | ||
| className="pl-9" | ||
| /> |
There was a problem hiding this comment.
Add an accessible label for the search input.
The search field at Line [94] has no associated label (placeholder is not a label), which hurts screen-reader usability.
♿ Minimal fix
- <div className="relative flex-1">
+ <div className="relative flex-1">
+ <label htmlFor="shop-search" className="sr-only">
+ Search products
+ </label>
<SearchIcon className="absolute left-3 top-1/2 -translate-y-1/2 w-4 h-4 text-shop-muted" />
<ShopInput
+ id="shop-search"
type="search"
value={inputValue}
onChange={(e) => setInputValue(e.target.value)}
placeholder="Search products…"
className="pl-9"
/>
</div>📝 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.
Suggested change
| <div className="relative flex-1"> | |
| <SearchIcon className="absolute left-3 top-1/2 -translate-y-1/2 w-4 h-4 text-gray-400" /> | |
| <input | |
| <SearchIcon className="absolute left-3 top-1/2 -translate-y-1/2 w-4 h-4 text-shop-muted" /> | |
| <ShopInput | |
| type="search" | |
| value={inputValue} | |
| onChange={(e) => setInputValue(e.target.value)} | |
| placeholder="Search products…" | |
| className="w-full pl-9 pr-3 py-2 rounded-lg border border-gray-300 dark:border-gray-700 bg-white dark:bg-gray-950 text-sm" | |
| className="pl-9" | |
| /> | |
| <div className="relative flex-1"> | |
| <label htmlFor="shop-search" className="sr-only"> | |
| Search products | |
| </label> | |
| <SearchIcon className="absolute left-3 top-1/2 -translate-y-1/2 w-4 h-4 text-shop-muted" /> | |
| <ShopInput | |
| id="shop-search" | |
| type="search" | |
| value={inputValue} | |
| onChange={(e) => setInputValue(e.target.value)} | |
| placeholder="Search products…" | |
| className="pl-9" | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/shop.search.tsx` around lines 92 - 100, The search input lacks an
accessible label; update the ShopInput usage to include an accessible label by
either adding a visible <label> with htmlFor matching an id prop on ShopInput
(e.g., give ShopInput id="shop-search" and add a visually-hidden label text
"Search products") or at minimum add an aria-label="Search products" prop to
ShopInput so screen readers can announce the field; ensure the change is applied
where SearchIcon, ShopInput, inputValue and setInputValue are used so the input
retains its id/aria-label and behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/shop: landing, PDP, cart, cart drawer, collection/search/policy/pages — editorial direction with cyan accent, DM Sans display, JetBrains Mono for prices/meta.shop-scopewrapper (light + dark), exposed to Tailwind via@theme inlineso authoring is idiomatic utility classes (bg-shop-panel,text-shop-muted,font-shop-display, etc.)src/components/shop/ui/— Button, Badge, Chip, Size, Qty, Select, Input, Tab, Panel, Pulse, Mono, Crumb — and composed pieces (Hero, DropCard, Strip, Specs, Note)shop.css+ Google Fonts only load on/shopProductListItemgainsproductTypeandtagsto drive the hover tag + New/Low-stock badgesStats
shop.css: 78 lines (tokens + one@keyframes).shop-*class selectors — all styling via Tailwind utilitiesstyle={{}}attributesTest plan
/shoplanding: hero, drop card, filter tabs (by productType), sort, product gridProductListItemconsumers (NavbarCartButton viacartDrawerStore) unaffectedbg-shop-*/font-shop-*utilities don't affect any non-shop pageNotes
--no-verify: the huskytest:smoketask requires siblingquery/router/tablerepos cloned next to the worktree — not present locally. Theformat+test:tsc+test:lintparts of the hook pass cleanly.Summary by CodeRabbit
Release Notes
New Features
Style