Merch Shop / DS Update - New Social logos - #1105
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
🚧 Files skipped from review as they are similar to previous changes (23)
📝 WalkthroughWalkthroughThe PR updates footer placement, responsive navigation, design-system navigation and controls, brand asset tooling, and shop tokens, product cards, and product drawer interactions. ChangesSite shell and navigation
Design-system navigation and components
Shop presentation and product flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant DsSidebar
participant RouterLocation
participant DsSection
Reviewer->>DsSidebar: Select subsection link
DsSidebar->>RouterLocation: Update URL hash
DsSidebar->>DsSection: Scroll to generated section ID
DsSection-->>Reviewer: Show selected subsection
sequenceDiagram
participant ProductDrawer
participant ReactQuery
participant ProductContent
participant ShopControls
ProductDrawer->>ReactQuery: Prefetch neighboring products
ReactQuery-->>ProductDrawer: Return cached product data
ProductDrawer->>ProductContent: Preserve and animate product content
ProductContent->>ShopControls: Render shared product controls
ShopControls-->>ProductDrawer: Report selection and cart actions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
tanstack-com | 86b457c | Commit Preview URL Branch Preview URL |
Aug 03 2026, 08:21 PM |
…shop-improvements # Conflicts: # src/components/MaintainerCard.tsx # src/components/Navbar.tsx # src/components/NavbarAuthControls.tsx # src/components/ds/BrandAssets.tsx # src/components/ds/ui/index.tsx # src/routes/blog.index.tsx # src/routes/maintainers.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/shop/ProductCard.tsx (1)
20-34: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the collection-level "New" badge on collections routes.
ProductCarddefaultsisNewtofalse, andshop.collections.$handle.tsxcalls it without that prop; only-shop-browse.tsxcomputes one product asnewestProductand passes it. If products shown underproduct.publishedAtshould show "New", compute that value in the collection route with the same published-date logic before renderingProductCard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/shop/ProductCard.tsx` around lines 20 - 34, Update the collection route’s ProductCard rendering to compute isNew using the same published-date logic as newestProduct in -shop-browse.tsx, based on product.publishedAt, and pass that value explicitly so the collection-level “New” badge is preserved.
🧹 Nitpick comments (3)
src/components/ds/BrandAssets.tsx (1)
197-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared asset-URL logic.
BrandLogoPickerandSocialLogoPickerduplicate the same format-to-extension mapping and the sameassetName/hrefcomposition.CopyAssetButtonand the downloadButtonblocks are also near-identical between the two pickers. A small shared helper for the extension plus a sharedAssetActionscomponent would remove the duplication and keep the two pickers in sync when a new format is added.Also applies to: 396-404
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ds/BrandAssets.tsx` around lines 197 - 212, Extract the duplicated format-to-extension and assetName/href construction used by BrandLogoPicker and SocialLogoPicker into shared helpers, then introduce a shared AssetActions component for the duplicated CopyAssetButton and download Button blocks. Update both pickers to use these shared symbols so future format additions and action behavior remain synchronized.src/components/ButtonGroup.tsx (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Tailwind v4 direct-child and
aria-*variants.Replace the arbitrary variant with
*:aria-pressed:border-transparent *:aria-pressed:bg-text-primary *:aria-pressed:text-background-default *:aria-pressed:shadow-smfor a cleaner supported selector when applying these styles to pressed children.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ButtonGroup.tsx` at line 18, Update the pressed-child class variants in ButtonGroup to use Tailwind v4 direct-child aria variants: replace each [&>[aria-pressed=true]] prefix with *:aria-pressed while preserving the existing border, background, text, and shadow utilities.src/components/shop/ProductDrawer.tsx (1)
645-656: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSanitize
descriptionHtmlbefore injecting it.
dangerouslySetInnerHTML={{ __html: product.descriptionHtml }}renders Shopify product description HTML without sanitization. This bypasses React's built-in XSS protection. If the description content is ever edited by a lower-trust staff account or the CMS content is compromised, this becomes a stored-XSS vector.Sanitize
product.descriptionHtmlwith a library such as DOMPurify before rendering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/shop/ProductDrawer.tsx` around lines 645 - 656, Sanitize product.descriptionHtml with the project’s approved HTML sanitization library, such as DOMPurify, before passing it to dangerouslySetInnerHTML in the product description block of ProductDrawer. Keep the existing conditional rendering and styling unchanged, and inject only the sanitized HTML.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ds/BrandAssets.tsx`:
- Around line 133-161: Update the image-loading effect in DissolvingLogoPreview
by adding an image.onerror handler alongside image.onload. After confirming the
request is still active, commit the requested {src, alt} to current and clear
any incoming image/transition state so the preview matches the selected asset
even when loading fails.
In `@src/components/ds/ui/index.tsx`:
- Around line 503-522: Update the input props and non-progressive search-field
rendering around the visible `progressive` branch to provide a default
accessible `aria-label` for the search input, while allowing callers to override
it. Ensure the default is used when no label is supplied and preserve any
explicitly provided `aria-label`.
In `@src/components/MaintainerCard.tsx`:
- Line 226: Update the card variant social-links layout class to use
justify-start instead of justify-center, aligning its icons with the
left-aligned content while preserving the existing wrapping, centering, and gap
classes.
In `@src/components/shop/ProductDrawer.tsx`:
- Around line 128-132: Replace the render-time mutations of hasOpened in
ProductDrawer with state and the established derived-state-during-render pattern
used nearby. Update the state when isOpen and prefetchedProduct indicate the
first open, reset it when the drawer closes without displayHandle, and derive
isAnimatedOpen from the state while preserving the existing visibility behavior.
In `@src/routes/__root.tsx`:
- Around line 323-330: Update the non-hideNavbar branch in the route component
so the children are wrapped in a flex-growing element before rendering Footer,
allowing short route content to consume remaining viewport height and keeping
the footer at the bottom. Leave the hideNavbar branch unchanged.
In `@src/routes/ds.avatar.tsx`:
- Around line 91-98: Align the third responsive grid breakpoint between the
ComponentPreview className and its code sample, updating the mismatched xl/lg
utility so both use the same breakpoint while preserving the other grid classes.
In `@src/routes/ds.buttons.tsx`:
- Around line 286-292: Update the documented ButtonPreview snippet in the
surrounding component to use the same capitalized period values as PERIODS and
SegmentedButtonGroup: “Day”, “Week”, and “Month”. Keep the aria-pressed
comparisons aligned with the actual period values.
In `@src/routes/ds.maintainers.tsx`:
- Around line 3-7: Update the redirect options in the Route beforeLoad handler
to set replace: true, ensuring the redirect from /ds/maintainers replaces its
history entry while preserving the existing destination and hash.
In `@src/routes/ds.tsx`:
- Around line 159-171: Update the subsection navigation around the sidebar link
and navigateToSection: in src/routes/ds.tsx lines 159-171, replace the plain
anchor with the router Link using item.to and the subsection hash, retaining the
custom scroll handler only for same-page navigation; in src/routes/ds.tsx lines
46-47, replace window.history.pushState with router navigation that sets the
hash so router location and the address bar remain synchronized.
In `@src/styles/shop.css`:
- Around line 42-51: Update the background-color declaration in .shop-merch-mark
to use the lowercase CSS keyword currentcolor, preserving the existing styling
and other declarations.
---
Outside diff comments:
In `@src/components/shop/ProductCard.tsx`:
- Around line 20-34: Update the collection route’s ProductCard rendering to
compute isNew using the same published-date logic as newestProduct in
-shop-browse.tsx, based on product.publishedAt, and pass that value explicitly
so the collection-level “New” badge is preserved.
---
Nitpick comments:
In `@src/components/ButtonGroup.tsx`:
- Line 18: Update the pressed-child class variants in ButtonGroup to use
Tailwind v4 direct-child aria variants: replace each [&>[aria-pressed=true]]
prefix with *:aria-pressed while preserving the existing border, background,
text, and shadow utilities.
In `@src/components/ds/BrandAssets.tsx`:
- Around line 197-212: Extract the duplicated format-to-extension and
assetName/href construction used by BrandLogoPicker and SocialLogoPicker into
shared helpers, then introduce a shared AssetActions component for the
duplicated CopyAssetButton and download Button blocks. Update both pickers to
use these shared symbols so future format additions and action behavior remain
synchronized.
In `@src/components/shop/ProductDrawer.tsx`:
- Around line 645-656: Sanitize product.descriptionHtml with the project’s
approved HTML sanitization library, such as DOMPurify, before passing it to
dangerouslySetInnerHTML in the product description block of ProductDrawer. Keep
the existing conditional rendering and styling unchanged, and inject only the
sanitized HTML.
🪄 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 Plus
Run ID: dc0f9a33-ff51-4b12-828c-9ffed09e7ea1
⛔ Files ignored due to path filters (52)
public/android-chrome-192x192.pngis excluded by!**/*.pngpublic/android-chrome-512x512.pngis excluded by!**/*.pngpublic/apple-touch-icon.pngis excluded by!**/*.pngpublic/favicon-16x16.pngis excluded by!**/*.pngpublic/favicon-32x32.pngis excluded by!**/*.pngpublic/favicon-dark.svgis excluded by!**/*.svgpublic/favicon-light.svgis excluded by!**/*.svgpublic/favicon.icois excluded by!**/*.icopublic/favicon.pngis excluded by!**/*.pngpublic/images/brand/social/mark-green.svgis excluded by!**/*.svgpublic/images/brand/social/mark-green@2x.pngis excluded by!**/*.pngpublic/images/brand/social/mark-ocean.svgis excluded by!**/*.svgpublic/images/brand/social/mark-ocean@2x.pngis excluded by!**/*.pngpublic/images/brand/social/mark-purple.svgis excluded by!**/*.svgpublic/images/brand/social/mark-purple@2x.pngis excluded by!**/*.pngpublic/images/brand/social/mark-yellow.svgis excluded by!**/*.svgpublic/images/brand/social/mark-yellow@2x.pngis excluded by!**/*.pngpublic/images/brand/social/naked-mark-dark.svgis excluded by!**/*.svgpublic/images/brand/social/naked-mark-dark@2x.pngis excluded by!**/*.pngpublic/images/brand/social/naked-mark-green.svgis excluded by!**/*.svgpublic/images/brand/social/naked-mark-green@2x.pngis excluded by!**/*.pngpublic/images/brand/social/naked-mark-light.svgis excluded by!**/*.svgpublic/images/brand/social/naked-mark-light@2x.pngis excluded by!**/*.pngpublic/images/brand/social/naked-mark-ocean.svgis excluded by!**/*.svgpublic/images/brand/social/naked-mark-ocean@2x.pngis excluded by!**/*.pngpublic/images/brand/social/naked-mark-purple.svgis excluded by!**/*.svgpublic/images/brand/social/naked-mark-purple@2x.pngis excluded by!**/*.pngpublic/images/brand/social/naked-mark-yellow.svgis excluded by!**/*.svgpublic/images/brand/social/naked-mark-yellow@2x.pngis excluded by!**/*.pngpublic/images/brand/social/stacked-dark.svgis excluded by!**/*.svgpublic/images/brand/social/stacked-dark@2x.pngis excluded by!**/*.pngpublic/images/brand/social/stacked-green.svgis excluded by!**/*.svgpublic/images/brand/social/stacked-green@2x.pngis excluded by!**/*.pngpublic/images/brand/social/stacked-light.svgis excluded by!**/*.svgpublic/images/brand/social/stacked-light@2x.pngis excluded by!**/*.pngpublic/images/brand/social/stacked-ocean.svgis excluded by!**/*.svgpublic/images/brand/social/stacked-ocean@2x.pngis excluded by!**/*.pngpublic/images/brand/social/stacked-purple.svgis excluded by!**/*.svgpublic/images/brand/social/stacked-purple@2x.pngis excluded by!**/*.pngpublic/images/brand/social/stacked-yellow.svgis excluded by!**/*.svgpublic/images/brand/social/stacked-yellow@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-emblem-black@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-emblem-charcoal@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-emblem-cream@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-emblem-white@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-landscape-black@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-landscape-charcoal@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-landscape-white@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-stacked-black@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-stacked-charcoal@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-stacked-cream@2x.pngis excluded by!**/*.pngpublic/images/brand/tanstack-stacked-white@2x.pngis excluded by!**/*.png
📒 Files selected for processing (54)
src/components/ButtonGroup.tsxsrc/components/Footer.tsxsrc/components/LibraryLayout.tsxsrc/components/MaintainerCard.tsxsrc/components/Navbar.tsxsrc/components/NavbarAuthControls.tsxsrc/components/ds/BrandAssets.tsxsrc/components/ds/DsKit.tsxsrc/components/ds/ds-nav.tssrc/components/ds/ui/index.tsxsrc/components/home/HomeNewsletterSection.tsxsrc/components/shop/CartDrawer.tsxsrc/components/shop/ProductCard.tsxsrc/components/shop/ProductDrawer.tsxsrc/components/shop/ShopDropCard.tsxsrc/components/shop/ShopNote.tsxsrc/components/shop/ShopSpecs.tsxsrc/components/shop/ShopStrip.tsxsrc/components/shop/ui/Badge.tsxsrc/components/shop/ui/Chip.tsxsrc/components/shop/ui/Crumb.tsxsrc/components/shop/ui/Input.tsxsrc/components/shop/ui/Qty.tsxsrc/components/shop/ui/Select.tsxsrc/routes/-shop-browse.tsxsrc/routes/__root.tsxsrc/routes/ads.tsxsrc/routes/blog.index.tsxsrc/routes/ds.avatar.tsxsrc/routes/ds.buttons.tsxsrc/routes/ds.eyebrow.tsxsrc/routes/ds.inputs.tsxsrc/routes/ds.maintainers.tsxsrc/routes/ds.tsxsrc/routes/ethos.tsxsrc/routes/index.tsxsrc/routes/maintainers.tsxsrc/routes/merch.tsxsrc/routes/partners.$partner.tsxsrc/routes/partners.index.tsxsrc/routes/partners.railway.tsxsrc/routes/privacy.tsxsrc/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/tenets.tsxsrc/routes/terms.tsxsrc/routes/workshops.tsxsrc/styles/app.csssrc/styles/shop.csssrc/utils/shop-color.ts
💤 Files with no reviewable changes (14)
- src/routes/partners.index.tsx
- src/components/home/HomeNewsletterSection.tsx
- src/components/LibraryLayout.tsx
- src/routes/tenets.tsx
- src/routes/terms.tsx
- src/routes/ethos.tsx
- src/routes/maintainers.tsx
- src/routes/partners.$partner.tsx
- src/routes/privacy.tsx
- src/routes/merch.tsx
- src/routes/ads.tsx
- src/routes/blog.index.tsx
- src/routes/workshops.tsx
- src/routes/partners.railway.tsx
| React.useEffect(() => { | ||
| if (src === current.src) return | ||
|
|
||
| const activeRequest = ++requestId.current | ||
| const image = new Image() | ||
| image.onload = () => { | ||
| if (activeRequest !== requestId.current) return | ||
|
|
||
| if (window.matchMedia('(prefers-reduced-motion: reduce)').matches) { | ||
| setCurrent({ src, alt }) | ||
| setIncoming(null) | ||
| setIncomingVisible(false) | ||
| return | ||
| } | ||
|
|
||
| setIncoming({ src, alt }) | ||
| setIncomingVisible(false) | ||
| window.requestAnimationFrame(() => { | ||
| window.requestAnimationFrame(() => { | ||
| if (activeRequest === requestId.current) setIncomingVisible(true) | ||
| }) | ||
| }) | ||
| } | ||
| image.src = src | ||
|
|
||
| return () => { | ||
| requestId.current += 1 | ||
| } | ||
| }, [alt, current.src, src]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle image load failures in DissolvingLogoPreview.
The effect only sets image.onload. If the requested asset fails to load, no state update occurs. The preview then keeps the previous logo while the footer label, the copy action, and the download link already point to the new selection. The user sees a mismatch with no feedback.
Add an onerror handler that commits the new source, so the broken-image state matches the labels.
🛠️ Proposed fix
const activeRequest = ++requestId.current
const image = new Image()
image.onload = () => {
if (activeRequest !== requestId.current) return
if (window.matchMedia('(prefers-reduced-motion: reduce)').matches) {
setCurrent({ src, alt })
setIncoming(null)
setIncomingVisible(false)
return
}
setIncoming({ src, alt })
setIncomingVisible(false)
window.requestAnimationFrame(() => {
window.requestAnimationFrame(() => {
if (activeRequest === requestId.current) setIncomingVisible(true)
})
})
}
+ image.onerror = () => {
+ if (activeRequest !== requestId.current) return
+ setCurrent({ src, alt })
+ setIncoming(null)
+ setIncomingVisible(false)
+ }
image.src = src📝 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.
| React.useEffect(() => { | |
| if (src === current.src) return | |
| const activeRequest = ++requestId.current | |
| const image = new Image() | |
| image.onload = () => { | |
| if (activeRequest !== requestId.current) return | |
| if (window.matchMedia('(prefers-reduced-motion: reduce)').matches) { | |
| setCurrent({ src, alt }) | |
| setIncoming(null) | |
| setIncomingVisible(false) | |
| return | |
| } | |
| setIncoming({ src, alt }) | |
| setIncomingVisible(false) | |
| window.requestAnimationFrame(() => { | |
| window.requestAnimationFrame(() => { | |
| if (activeRequest === requestId.current) setIncomingVisible(true) | |
| }) | |
| }) | |
| } | |
| image.src = src | |
| return () => { | |
| requestId.current += 1 | |
| } | |
| }, [alt, current.src, src]) | |
| React.useEffect(() => { | |
| if (src === current.src) return | |
| const activeRequest = ++requestId.current | |
| const image = new Image() | |
| image.onload = () => { | |
| if (activeRequest !== requestId.current) return | |
| if (window.matchMedia('(prefers-reduced-motion: reduce)').matches) { | |
| setCurrent({ src, alt }) | |
| setIncoming(null) | |
| setIncomingVisible(false) | |
| return | |
| } | |
| setIncoming({ src, alt }) | |
| setIncomingVisible(false) | |
| window.requestAnimationFrame(() => { | |
| window.requestAnimationFrame(() => { | |
| if (activeRequest === requestId.current) setIncomingVisible(true) | |
| }) | |
| }) | |
| } | |
| image.onerror = () => { | |
| if (activeRequest !== requestId.current) return | |
| setCurrent({ src, alt }) | |
| setIncoming(null) | |
| setIncomingVisible(false) | |
| } | |
| image.src = src | |
| return () => { | |
| requestId.current += 1 | |
| } | |
| }, [alt, current.src, src]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ds/BrandAssets.tsx` around lines 133 - 161, Update the
image-loading effect in DissolvingLogoPreview by adding an image.onerror handler
alongside image.onload. After confirming the request is still active, commit the
requested {src, alt} to current and clear any incoming image/transition state so
the preview matches the selected asset even when loading fails.
| if (!progressive) { | ||
| return ( | ||
| <label | ||
| className={twMerge( | ||
| 'flex w-full items-center border border-border-default bg-background-surface text-text-muted transition-[border-color,box-shadow] duration-150 focus-within:border-border-focus focus-within:text-text-primary focus-within:ring-2 focus-within:ring-border-focus/40 motion-reduce:transition-none', | ||
| size === 'large' | ||
| ? 'min-h-14 gap-3 rounded-xl px-4' | ||
| : 'h-10 gap-2.5 rounded-lg px-3', | ||
| )} | ||
| > | ||
| <MagnifyingGlass | ||
| size={size === 'large' ? 21 : 18} | ||
| weight="bold" | ||
| aria-hidden="true" | ||
| className="shrink-0" | ||
| /> | ||
| {input} | ||
| </label> | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Give the search field an accessible name.
The non-progressive branch wraps the input in a <label> that contains only the decorative icon. The icon carries aria-hidden="true", so the label exposes no text. Screen-reader users then depend on the placeholder for the accessible name, which is not a reliable label. The ds.inputs.tsx examples pass only placeholder.
Set a default aria-label on the input so every consumer gets a name, and allow callers to override it.
♿ Proposed fix
const input = (
<input
ref={inputRef}
type="search"
+ aria-label={props['aria-label'] ?? 'Search'}
className={twMerge(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ds/ui/index.tsx` around lines 503 - 522, Update the input
props and non-progressive search-field rendering around the visible
`progressive` branch to provide a default accessible `aria-label` for the search
input, while allowing callers to override it. Ensure the default is used when no
label is supplied and preserve any explicitly provided `aria-label`.
| className={ | ||
| variant === 'card' | ||
| ? 'flex flex-wrap items-center justify-center gap-6' | ||
| ? 'flex flex-wrap items-center justify-center gap-4 sm:gap-6' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the card social links with the left-aligned content.
Line 409 sets the content column to items-start and Line 411 sets the name to text-left. The card variant row still uses justify-center, so the icons center under a left-aligned name. Use justify-start for the card variant.
🎨 Proposed fix
- ? 'flex flex-wrap items-center justify-center gap-4 sm:gap-6'
+ ? 'flex flex-wrap items-center justify-start gap-4 sm:gap-6'📝 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.
| ? 'flex flex-wrap items-center justify-center gap-4 sm:gap-6' | |
| ? 'flex flex-wrap items-center justify-start gap-4 sm:gap-6' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/MaintainerCard.tsx` at line 226, Update the card variant
social-links layout class to use justify-start instead of justify-center,
aligning its icons with the left-aligned content while preserving the existing
wrapping, centering, and gap classes.
| // Wait for data on the first open, but keep the sheet visible while products swap. | ||
| const hasOpened = React.useRef(false) | ||
| if (isOpen && prefetchedProduct) hasOpened.current = true | ||
| if (!isOpen && !displayHandle) hasOpened.current = false | ||
| const isAnimatedOpen = isOpen && hasOpened.current |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ref mutated during render can leak stale state across discarded renders.
hasOpened.current is written directly in the render body (lines 130-131), not inside an event handler or effect. React can discard or replay a render (for example under Strict Mode double-invocation or interrupted concurrent rendering), so a mutation performed during a render that never commits can still leak into subsequent renders. isAnimatedOpen at line 132 then derives from a ref value that may not correspond to the actually-committed render.
The file already uses the safe "derived state during render" pattern with setState at lines 89-92 and 122-125. Convert hasOpened to state using the same pattern for consistency and correctness.
🔧 Proposed fix using state instead of a mutated ref
- const hasOpened = React.useRef(false)
- if (isOpen && prefetchedProduct) hasOpened.current = true
- if (!isOpen && !displayHandle) hasOpened.current = false
- const isAnimatedOpen = isOpen && hasOpened.current
+ const [hasOpened, setHasOpened] = React.useState(false)
+ if (isOpen && prefetchedProduct && !hasOpened) setHasOpened(true)
+ if (!isOpen && !displayHandle && hasOpened) setHasOpened(false)
+ const isAnimatedOpen = isOpen && hasOpened📝 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.
| // Wait for data on the first open, but keep the sheet visible while products swap. | |
| const hasOpened = React.useRef(false) | |
| if (isOpen && prefetchedProduct) hasOpened.current = true | |
| if (!isOpen && !displayHandle) hasOpened.current = false | |
| const isAnimatedOpen = isOpen && hasOpened.current | |
| // Wait for data on the first open, but keep the sheet visible while products swap. | |
| const [hasOpened, setHasOpened] = React.useState(false) | |
| if (isOpen && prefetchedProduct && !hasOpened) setHasOpened(true) | |
| if (!isOpen && !displayHandle && hasOpened) setHasOpened(false) | |
| const isAnimatedOpen = isOpen && hasOpened |
🧰 Tools
🪛 React Doctor (0.9.1)
[error] 130-130: This ref is mutated during render. React can replay or discard render work, so the mutation can leak from UI that never commits.
Move ref writes into an event handler or effect. Render must stay pure because React can replay or discard it. The predictable null-guarded lazy initialization pattern remains supported.
(no-ref-current-in-render)
[error] 131-131: This ref is mutated during render. React can replay or discard render work, so the mutation can leak from UI that never commits.
Move ref writes into an event handler or effect. Render must stay pure because React can replay or discard it. The predictable null-guarded lazy initialization pattern remains supported.
(no-ref-current-in-render)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/shop/ProductDrawer.tsx` around lines 128 - 132, Replace the
render-time mutations of hasOpened in ProductDrawer with state and the
established derived-state-during-render pattern used nearby. Update the state
when isOpen and prefetchedProduct indicate the first open, reset it when the
drawer closes without displayHandle, and derive isAnimatedOpen from the state
while preserving the existing visibility behavior.
Source: Linters/SAST tools
| {hideNavbar ? ( | ||
| children | ||
| ) : ( | ||
| <Navbar> | ||
| {children} | ||
| <Footer /> | ||
| </Navbar> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the footer at the bottom of short routes.
Navbar renders its children in a flex-1 flex flex-col wrapper at src/components/Navbar.tsx lines 740-742. The footer follows short route content inside that wrapper, so unused viewport height remains below the footer. Wrap the route content in a flex-growing element before Footer.
Proposed fix
<Navbar>
- {children}
+ <div className="flex flex-1 flex-col">
+ {children}
+ </div>
<Footer />
</Navbar>📝 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.
| {hideNavbar ? ( | |
| children | |
| ) : ( | |
| <Navbar> | |
| {children} | |
| <Footer /> | |
| </Navbar> | |
| )} | |
| {hideNavbar ? ( | |
| children | |
| ) : ( | |
| <Navbar> | |
| <div className="flex flex-1 flex-col"> | |
| {children} | |
| </div> | |
| <Footer /> | |
| </Navbar> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/__root.tsx` around lines 323 - 330, Update the non-hideNavbar
branch in the route component so the children are wrapped in a flex-growing
element before rendering Footer, allowing short route content to consume
remaining viewport height and keeping the footer at the bottom. Leave the
hideNavbar branch unchanged.
| <ComponentPreview | ||
| className="grid justify-items-center gap-6 sm:grid-cols-2 xl:grid-cols-3" | ||
| code={`<div className="grid grid-cols-1 gap-6 sm:grid-cols-2 lg:grid-cols-3"> | ||
| {maintainers.map((maintainer) => ( | ||
| <MaintainerCard key={maintainer.github} maintainer={maintainer} /> | ||
| ))} | ||
| </div>`} | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The documented snippet does not match the rendered grid.
The code sample uses grid-cols-1 gap-6 sm:grid-cols-2 lg:grid-cols-3. The rendered preview uses grid justify-items-center gap-6 sm:grid-cols-2 xl:grid-cols-3. A design-system page should show the classes it demonstrates, otherwise a reader copies a different breakpoint set than the one shown.
Align the third breakpoint in both places.
📝 Proposed fix
- code={`<div className="grid grid-cols-1 gap-6 sm:grid-cols-2 lg:grid-cols-3">
+ code={`<div className="grid grid-cols-1 gap-6 sm:grid-cols-2 xl:grid-cols-3">📝 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.
| <ComponentPreview | |
| className="grid justify-items-center gap-6 sm:grid-cols-2 xl:grid-cols-3" | |
| code={`<div className="grid grid-cols-1 gap-6 sm:grid-cols-2 lg:grid-cols-3"> | |
| {maintainers.map((maintainer) => ( | |
| <MaintainerCard key={maintainer.github} maintainer={maintainer} /> | |
| ))} | |
| </div>`} | |
| > | |
| <ComponentPreview | |
| className="grid justify-items-center gap-6 sm:grid-cols-2 xl:grid-cols-3" | |
| code={`<div className="grid grid-cols-1 gap-6 sm:grid-cols-2 xl:grid-cols-3"> | |
| {maintainers.map((maintainer) => ( | |
| <MaintainerCard key={maintainer.github} maintainer={maintainer} /> | |
| ))} | |
| </div>`} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/ds.avatar.tsx` around lines 91 - 98, Align the third responsive
grid breakpoint between the ComponentPreview className and its code sample,
updating the mismatched xl/lg utility so both use the same breakpoint while
preserving the other grid classes.
| <ButtonPreview | ||
| code={`<ButtonGroup> | ||
| <Button variant="ghost" rounded="none">Day</Button> | ||
| <Button variant="ghost" rounded="none">Week</Button> | ||
| <Button variant="ghost" rounded="none">Month</Button> | ||
| <Button variant="ghost" rounded="none" aria-pressed={period === 'day'}>Day</Button> | ||
| <Button variant="ghost" rounded="none" aria-pressed={period === 'week'}>Week</Button> | ||
| <Button variant="ghost" rounded="none" aria-pressed={period === 'month'}>Month</Button> | ||
| </ButtonGroup>`} | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The documented snippet uses the wrong case for the period values.
The sample compares period === 'day', 'week', and 'month'. PERIODS at Line 87 holds 'Day', 'Week', and 'Month'. A reader who copies the snippet gets aria-pressed={false} on every button. Match the sample to SegmentedButtonGroup.
📝 Proposed fix
code={`<ButtonGroup>
- <Button variant="ghost" rounded="none" aria-pressed={period === 'day'}>Day</Button>
- <Button variant="ghost" rounded="none" aria-pressed={period === 'week'}>Week</Button>
- <Button variant="ghost" rounded="none" aria-pressed={period === 'month'}>Month</Button>
+ <Button variant="ghost" rounded="none" aria-pressed={period === 'Day'}>Day</Button>
+ <Button variant="ghost" rounded="none" aria-pressed={period === 'Week'}>Week</Button>
+ <Button variant="ghost" rounded="none" aria-pressed={period === 'Month'}>Month</Button>
</ButtonGroup>`}📝 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.
| <ButtonPreview | |
| code={`<ButtonGroup> | |
| <Button variant="ghost" rounded="none">Day</Button> | |
| <Button variant="ghost" rounded="none">Week</Button> | |
| <Button variant="ghost" rounded="none">Month</Button> | |
| <Button variant="ghost" rounded="none" aria-pressed={period === 'day'}>Day</Button> | |
| <Button variant="ghost" rounded="none" aria-pressed={period === 'week'}>Week</Button> | |
| <Button variant="ghost" rounded="none" aria-pressed={period === 'month'}>Month</Button> | |
| </ButtonGroup>`} | |
| > | |
| <ButtonPreview | |
| code={`<ButtonGroup> | |
| <Button variant="ghost" rounded="none" aria-pressed={period === 'Day'}>Day</Button> | |
| <Button variant="ghost" rounded="none" aria-pressed={period === 'Week'}>Week</Button> | |
| <Button variant="ghost" rounded="none" aria-pressed={period === 'Month'}>Month</Button> | |
| </ButtonGroup>`} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/ds.buttons.tsx` around lines 286 - 292, Update the documented
ButtonPreview snippet in the surrounding component to use the same capitalized
period values as PERIODS and SegmentedButtonGroup: “Day”, “Week”, and “Month”.
Keep the aria-pressed comparisons aligned with the actual period values.
| export const Route = createFileRoute('/ds/maintainers')({ | ||
| component: MaintainersPage, | ||
| head: () => ({ | ||
| meta: seo({ | ||
| title: 'Maintainers | TanStack Design System', | ||
| description: | ||
| 'The shared maintainer card used for core teams, instructors, and library maintainers across TanStack.', | ||
| }), | ||
| }), | ||
| beforeLoad: () => { | ||
| throw redirect({ to: '/ds/avatar', hash: 'maintainer-card' }) | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
TanStack Router redirect replace option beforeLoad history stack
💡 Result:
In TanStack Router, the redirect function used within beforeLoad accepts the same navigation options as the navigate function, including the replace option [1][2][3]. To control the browser history stack when performing a redirect, you can pass { replace: true } in the options object [1][2][4]. Setting replace to true will cause the router to replace the current entry in the history stack instead of pushing a new one, which is particularly useful for avoiding history pollution in authentication flows [1][2][4]. Example usage within a beforeLoad hook: throw redirect({ to: '/login', replace: true, // Replaces current history entry }) The redirect function, when used with throw: true (or simply thrown as an object, which is the standard pattern), allows for type-safe and declarative navigation triggered during the route loading process [5][6][7][3]. Because redirect extends NavigateOptions, all standard navigation features—such as params, search, and state management—are fully supported alongside the history-controlling replace property [3].
Citations:
- 1: https://tanstack.com/router/latest/docs/guide/authenticated-routes
- 2: https://tanstack.com/router/v1/docs/guide/authenticated-routes
- 3: https://tanstack.com/router/latest/docs/api/router/RedirectType.md
- 4: fix: correctly handle client-side vs server-side redirects with rewrites router#5330
- 5: https://tanstack.com/router/v1/docs/api/router/redirectFunction
- 6: https://github.com/TanStack/router/blob/a4154714/packages/router-core/src/redirect.ts
- 7: https://tanstack.com/router/latest/docs/api/router/redirectFunction.md
Set replace: true on the redirect.
This route throws a redirect from beforeLoad; without replace, the browser history keeps /ds/maintainers below /ds/avatar#maintainer-card, so Back can return to the same redirect source. Set replace: true to replace the beforeLoad history entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/ds.maintainers.tsx` around lines 3 - 7, Update the redirect
options in the Route beforeLoad handler to set replace: true, ensuring the
redirect from /ds/maintainers replaces its history entry while preserving the
existing destination and hash.
| <a | ||
| href={`${item.to}#${toDsSectionId(subsection)}`} | ||
| onClick={(event) => | ||
| navigateToSection( | ||
| event, | ||
| item.to, | ||
| toDsSectionId(subsection), | ||
| ) | ||
| } | ||
| className="block rounded-md px-2 py-1 text-xs text-text-muted hover:bg-background-subtle hover:text-text-primary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-border-focus" | ||
| > | ||
| {subsection} | ||
| </a> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The design-system sidebar navigates outside TanStack Router. The subsection links use a plain <a href> and the same-page handler writes the hash with window.history.pushState. Both bypass the router, so cross-page clicks trigger a full document load and the router's location state never learns the new hash. Route both paths through the router.
src/routes/ds.tsx#L159-L171: replace the<a href>with the routerLink, passingto={item.to}andhash={toDsSectionId(subsection)}, and keep the custom scroll handler only for the same-page case.src/routes/ds.tsx#L46-L47: replacewindow.history.pushState(null, '', '#' + sectionId)with a router navigation that sets the hash, souseLocation()and the address bar stay consistent.
📍 Affects 1 file
src/routes/ds.tsx#L159-L171(this comment)src/routes/ds.tsx#L46-L47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/ds.tsx` around lines 159 - 171, Update the subsection navigation
around the sidebar link and navigateToSection: in src/routes/ds.tsx lines
159-171, replace the plain anchor with the router Link using item.to and the
subsection hash, retaining the custom scroll handler only for same-page
navigation; in src/routes/ds.tsx lines 46-47, replace window.history.pushState
with router navigation that sets the hash so router location and the address bar
remain synchronized.
| .shop-merch-mark { | ||
| display: inline-block; | ||
| width: 0.82em; | ||
| height: 0.82em; | ||
| flex: none; | ||
| background-color: currentColor; | ||
| -webkit-mask: url('/images/brand/tanstack-emblem-black.svg') center / contain | ||
| no-repeat; | ||
| mask: url('/images/brand/tanstack-emblem-black.svg') center / contain | ||
| no-repeat; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the configured CSS keyword casing.
Line 47 fails Stylelint value-keyword-case. Replace currentColor with currentcolor.
Proposed fix
- background-color: currentColor;
+ background-color: currentcolor;📝 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.
| .shop-merch-mark { | |
| display: inline-block; | |
| width: 0.82em; | |
| height: 0.82em; | |
| flex: none; | |
| background-color: currentColor; | |
| -webkit-mask: url('/images/brand/tanstack-emblem-black.svg') center / contain | |
| no-repeat; | |
| mask: url('/images/brand/tanstack-emblem-black.svg') center / contain | |
| no-repeat; | |
| .shop-merch-mark { | |
| display: inline-block; | |
| width: 0.82em; | |
| height: 0.82em; | |
| flex: none; | |
| background-color: currentcolor; | |
| -webkit-mask: url('/images/brand/tanstack-emblem-black.svg') center / contain | |
| no-repeat; | |
| mask: url('/images/brand/tanstack-emblem-black.svg') center / contain | |
| no-repeat; |
🧰 Tools
🪛 Stylelint (17.14.1)
[error] 47-47: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/styles/shop.css` around lines 42 - 51, Update the background-color
declaration in .shop-merch-mark to use the lowercase CSS keyword currentcolor,
preserving the existing styling and other declarations.
Source: Linters/SAST tools
2 participants
Summary
Testing
Not run (not requested).
Summary by CodeRabbit
New Features
Improvements