fix: prevent multiple carousels from responding to arrow keys at the same time#752
Merged
tomaszpacior merged 3 commits intomainfrom Mar 9, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a managed keyboard navigation system so only the active Swiper-based carousel handles arrow keys. Introduces a global active-carousel manager, a useManagedCarouselKeyboard hook, prop surface for keyboardControlMode/keyboardCarouselId across Carousel/ProductCarousel/ProductGallery, and enables managed mode in ProductDetails and RecommendedProducts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main as Main Gallery<br/>(Carousel)
participant Manager as Carousel Keyboard<br/>Manager
participant Lightbox as Lightbox<br/>(Carousel)
participant SwiperMain as Swiper<br/>Instance (main)
participant SwiperLight as Swiper<br/>Instance (lightbox)
User->>Main: focus / pointer interaction
Main->>Manager: activateManagedKeyboard(mainId)
Manager->>Manager: setActiveCarouselId(mainId)
Manager-->>Main: notify active -> enable keyboard
Manager-->>Lightbox: notify inactive -> disable keyboard
User->>Main: press ArrowKey
Main->>SwiperMain: keyboard navigation
User->>Main: open lightbox
Lightbox->>Manager: activateManagedKeyboard(lightboxId)
Manager->>Manager: setActiveCarouselId(lightboxId)
Manager-->>Main: notify inactive -> disable keyboard
Manager-->>Lightbox: notify active -> enable keyboard
User->>Lightbox: press ArrowKey
Lightbox->>SwiperLight: keyboard navigation
User->>Lightbox: close
Lightbox->>Manager: clearActiveCarouselId(lightboxId)
Manager->>Manager: revert/clear active
Manager-->>Main: notify active -> enable keyboard
sequenceDiagram
participant Component as Carousel<br/>Component
participant Hook as useManagedCarousel<br/>Keyboard Hook
participant Manager as Global Carousel<br/>Manager
participant Swiper as Swiper<br/>Instance
Component->>Hook: init(managed, id, swiper)
Hook->>Manager: subscribeActiveCarousel()
Manager-->>Hook: current active id
Hook->>Hook: set local enabled state
Component->>Component: user focus/pointer
Component->>Hook: activateManagedKeyboard()
Hook->>Manager: setActiveCarouselId(id)
Manager->>Manager: notify subscribers
Manager-->>Hook: active id changed
Hook->>Swiper: enable or disable keyboard plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/ui/src/components/ProductGallery/ProductGallery.types.ts (1)
18-20: Consider usingCarouselPropstype references for consistency.Similar to
ProductCarousel.types.ts, these props could referenceCarouselProps['...']to ensure types stay synchronized with the source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/ProductGallery/ProductGallery.types.ts` around lines 18 - 20, Update the three prop types in ProductGallery.types.ts (keyboardControlMode, defaultKeyboardActive, keyboardCarouselId) to reference the canonical CarouselProps types instead of duplicating types; e.g., replace their standalone types with CarouselProps['keyboardControlMode'], CarouselProps['defaultKeyboardActive'] and CarouselProps['keyboardCarouselId'] so they stay synchronized with ProductCarousel's source-of-truth definitions.packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts (1)
17-19: Minor type reference inconsistency.
keyboardControlModecorrectly referencesCarouselProps['keyboardControlMode'], butdefaultKeyboardActiveandkeyboardCarouselIduse inline types. Consider usingCarouselProps['defaultKeyboardActive']andCarouselProps['keyboardCarouselId']for consistency and to ensure types stay in sync.♻️ Optional: Use consistent type references
keyboardControlMode?: CarouselProps['keyboardControlMode']; - defaultKeyboardActive?: boolean; - keyboardCarouselId?: string; + defaultKeyboardActive?: CarouselProps['defaultKeyboardActive']; + keyboardCarouselId?: CarouselProps['keyboardCarouselId'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts` around lines 17 - 19, The three props in ProductCarousel.types are inconsistent: while keyboardControlMode already uses CarouselProps['keyboardControlMode'], change defaultKeyboardActive and keyboardCarouselId to reference the corresponding types on CarouselProps as well (use CarouselProps['defaultKeyboardActive'] and CarouselProps['keyboardCarouselId']) so all three (keyboardControlMode, defaultKeyboardActive, keyboardCarouselId) are consistently typed against the CarouselProps definition.packages/ui/src/components/Carousel/carouselKeyboardManager.ts (1)
1-35: Consider adding'use client'directive for SSR safety.This module maintains singleton state via module-level variables. If accidentally imported in a Server Component context during SSR, the shared state could behave unexpectedly. Given that the consuming hook (
useManagedCarouselKeyboard.ts) already has'use client', this may be fine in practice, but adding the directive here would make the boundary explicit.The implementation is clean and follows a standard pub/sub pattern.
🛡️ Optional: Add client directive
+`use client`; + type ActiveCarouselListener = (activeCarouselId: string | null) => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Carousel/carouselKeyboardManager.ts` around lines 1 - 35, This module holds module-level singleton state (activeCarouselId, listeners) and should be explicitly marked as a client module to avoid accidental SSR import; add the 'use client' directive as the first statement in carouselKeyboardManager.ts so the module (and its exports: getActiveCarouselId, setActiveCarouselId, clearActiveCarouselId, subscribeActiveCarousel, notifyListeners) always runs on the client and won’t be bundled/executed during server rendering.packages/ui/src/components/ProductCarousel/ProductCarousel.tsx (1)
91-94: PreventcarouselConfigfrom unintentionally overriding explicit keyboard props.With current order,
carouselConfigwins overkeyboardControlMode,defaultKeyboardActive, andkeyboardCarouselId, which can be surprising for callers.♻️ Proposed refactor
- keyboardControlMode={keyboardControlMode} - defaultKeyboardActive={defaultKeyboardActive} - keyboardCarouselId={keyboardCarouselId} - {...carouselConfig} + {...carouselConfig} + keyboardControlMode={keyboardControlMode} + defaultKeyboardActive={defaultKeyboardActive} + keyboardCarouselId={keyboardCarouselId}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/ProductCarousel/ProductCarousel.tsx` around lines 91 - 94, The current JSX spreads carouselConfig after explicit keyboard props so its values can unintentionally override keyboardControlMode, defaultKeyboardActive, and keyboardCarouselId; update ProductCarousel JSX to spread {...carouselConfig} before passing the explicit props (keyboardControlMode, defaultKeyboardActive, keyboardCarouselId) so the explicit prop values win, ensuring callers' keyboard props are not overwritten by carouselConfig.packages/ui/src/components/ProductGallery/ProductGallery.tsx (1)
82-87: Use a lightbox-specific keyboard config at Line 229.Line 229 currently reuses the main gallery
keyboardConfig. Wiring the lightbox Swiper to the lightbox hook’s config keeps this path decoupled and less fragile to future hook changes.♻️ Proposed refactor
- const { activateManagedKeyboard: activateLightboxKeyboard } = useManagedCarouselKeyboard({ + const { activateManagedKeyboard: activateLightboxKeyboard, keyboardConfig: lightboxKeyboardConfig } = + useManagedCarouselKeyboard({ keyboardControlMode, carouselId: lightboxCarouselId, swiper: lightboxMainSwiper, shouldEnable: (activeCarouselId) => activeCarouselId === lightboxCarouselId && isLightboxOpen, - }); + }); ... - keyboard={keyboardConfig} + keyboard={lightboxKeyboardConfig}Also applies to: 229-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/ProductGallery/ProductGallery.tsx` around lines 82 - 87, The lightbox Swiper is currently wired to the main gallery's keyboardConfig, making it fragile; instead use the keyboard configuration returned by the lightbox-specific useManagedCarouselKeyboard hook (the value tied to activateManagedKeyboard: activateLightboxKeyboard from useManagedCarouselKeyboard) when wiring the lightbox Swiper (lightboxMainSwiper) so the lightbox path is decoupled — replace references to the shared keyboardConfig at the lightbox hook usage (around where lightboxMainSwiper is configured) with the keyboard config/activation provided by activateLightboxKeyboard and ensure shouldEnable uses lightboxCarouselId and isLightboxOpen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/components/Carousel/carouselKeyboardManager.ts`:
- Around line 1-35: This module holds module-level singleton state
(activeCarouselId, listeners) and should be explicitly marked as a client module
to avoid accidental SSR import; add the 'use client' directive as the first
statement in carouselKeyboardManager.ts so the module (and its exports:
getActiveCarouselId, setActiveCarouselId, clearActiveCarouselId,
subscribeActiveCarousel, notifyListeners) always runs on the client and won’t be
bundled/executed during server rendering.
In `@packages/ui/src/components/ProductCarousel/ProductCarousel.tsx`:
- Around line 91-94: The current JSX spreads carouselConfig after explicit
keyboard props so its values can unintentionally override keyboardControlMode,
defaultKeyboardActive, and keyboardCarouselId; update ProductCarousel JSX to
spread {...carouselConfig} before passing the explicit props
(keyboardControlMode, defaultKeyboardActive, keyboardCarouselId) so the explicit
prop values win, ensuring callers' keyboard props are not overwritten by
carouselConfig.
In `@packages/ui/src/components/ProductCarousel/ProductCarousel.types.ts`:
- Around line 17-19: The three props in ProductCarousel.types are inconsistent:
while keyboardControlMode already uses CarouselProps['keyboardControlMode'],
change defaultKeyboardActive and keyboardCarouselId to reference the
corresponding types on CarouselProps as well (use
CarouselProps['defaultKeyboardActive'] and CarouselProps['keyboardCarouselId'])
so all three (keyboardControlMode, defaultKeyboardActive, keyboardCarouselId)
are consistently typed against the CarouselProps definition.
In `@packages/ui/src/components/ProductGallery/ProductGallery.tsx`:
- Around line 82-87: The lightbox Swiper is currently wired to the main
gallery's keyboardConfig, making it fragile; instead use the keyboard
configuration returned by the lightbox-specific useManagedCarouselKeyboard hook
(the value tied to activateManagedKeyboard: activateLightboxKeyboard from
useManagedCarouselKeyboard) when wiring the lightbox Swiper (lightboxMainSwiper)
so the lightbox path is decoupled — replace references to the shared
keyboardConfig at the lightbox hook usage (around where lightboxMainSwiper is
configured) with the keyboard config/activation provided by
activateLightboxKeyboard and ensure shouldEnable uses lightboxCarouselId and
isLightboxOpen.
In `@packages/ui/src/components/ProductGallery/ProductGallery.types.ts`:
- Around line 18-20: Update the three prop types in ProductGallery.types.ts
(keyboardControlMode, defaultKeyboardActive, keyboardCarouselId) to reference
the canonical CarouselProps types instead of duplicating types; e.g., replace
their standalone types with CarouselProps['keyboardControlMode'],
CarouselProps['defaultKeyboardActive'] and CarouselProps['keyboardCarouselId']
so they stay synchronized with ProductCarousel's source-of-truth definitions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.changeset/wet-paws-cough.mdpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/blocks/recommended-products/src/frontend/RecommendedProducts.client.tsxpackages/ui/src/components/Carousel/Carousel.tsxpackages/ui/src/components/Carousel/Carousel.types.tspackages/ui/src/components/Carousel/carouselKeyboardManager.tspackages/ui/src/components/Carousel/useManagedCarouselKeyboard.tspackages/ui/src/components/ProductCarousel/ProductCarousel.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.types.tspackages/ui/src/components/ProductGallery/ProductGallery.tsxpackages/ui/src/components/ProductGallery/ProductGallery.types.ts
Contributor
Coverage Report for packages/configs/vitest-config
File CoverageNo changed files found. |
marcinkrasowski
requested changes
Feb 27, 2026
Remove default keyboard-active behavior so arrow keys stay native until a carousel is focused or clicked. Auto-generate keyboardCarouselId in ProductCarousel when missing to reduce manual prop wiring. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/ui/src/components/Carousel/useManagedCarouselKeyboard.ts (1)
63-72: Consider documenting theenabled: falseinitial state for managed mode.When
keyboardControlMode === 'managed', this returnsenabled: false(since'managed' !== 'swiper-native'). The keyboard is then toggled viaswiper.keyboard.enable()/disable()in the effect above. This is correct but non-obvious—a brief inline comment would help future maintainers understand that managed mode intentionally starts disabled and is controlled imperatively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Carousel/useManagedCarouselKeyboard.ts` around lines 63 - 72, The keyboardConfig computed in useManagedCarouselKeyboard currently sets enabled: keyboardControlMode === 'swiper-native', which means managed mode intentionally starts with enabled: false and is toggled later via swiper.keyboard.enable()/disable(); add a brief inline comment next to the keyboardConfig (or above the useMemo) explaining that when keyboardControlMode === 'managed' the config intentionally initializes as disabled and that the effect uses swiper.keyboard.enable()/disable() to control it imperatively so future maintainers understand the rationale.packages/blocks/product-details/src/frontend/ProductDetails.client.tsx (1)
138-138: Explicit but redundant prop value.
ProductGalleryalready defaults tokeyboardControlMode="managed"(seeProductGallery.tsxline 30). This explicit setting is redundant but documents intent clearly at the call site. Consider removing it for brevity, or keep it for explicit documentation—either approach is valid.This addresses the past review comment asking about defaults:
'managed'is indeed the default forProductGallery, so no explicit prop is strictly required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/blocks/product-details/src/frontend/ProductDetails.client.tsx` at line 138, The call to ProductGallery includes the explicit prop keyboardControlMode="managed" which is redundant because ProductGallery defaults to 'managed'; remove the explicit keyboardControlMode prop from the ProductGallery JSX at the call site to clean up the code (or if you prefer to keep it for clarity, you may leave it—choose one approach and make it consistent across usages of ProductGallery).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/ProductCarousel/ProductCarousel.tsx`:
- Around line 93-95: The prop spread ordering currently allows carouselConfig to
override the explicit props keyboardControlMode and keyboardCarouselId; update
the JSX where keyboardControlMode and keyboardCarouselId are passed (the
component call using keyboardControlMode={keyboardControlMode}
keyboardCarouselId={resolvedKeyboardCarouselId} {...carouselConfig}) so that
{...carouselConfig} appears before the explicit props, ensuring the explicit
keyboardControlMode and keyboardCarouselId values win when both are provided.
---
Nitpick comments:
In `@packages/blocks/product-details/src/frontend/ProductDetails.client.tsx`:
- Line 138: The call to ProductGallery includes the explicit prop
keyboardControlMode="managed" which is redundant because ProductGallery defaults
to 'managed'; remove the explicit keyboardControlMode prop from the
ProductGallery JSX at the call site to clean up the code (or if you prefer to
keep it for clarity, you may leave it—choose one approach and make it consistent
across usages of ProductGallery).
In `@packages/ui/src/components/Carousel/useManagedCarouselKeyboard.ts`:
- Around line 63-72: The keyboardConfig computed in useManagedCarouselKeyboard
currently sets enabled: keyboardControlMode === 'swiper-native', which means
managed mode intentionally starts with enabled: false and is toggled later via
swiper.keyboard.enable()/disable(); add a brief inline comment next to the
keyboardConfig (or above the useMemo) explaining that when keyboardControlMode
=== 'managed' the config intentionally initializes as disabled and that the
effect uses swiper.keyboard.enable()/disable() to control it imperatively so
future maintainers understand the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46d7082a-82b4-48bf-aace-5ed2d2c30fb3
📒 Files selected for processing (10)
.changeset/wet-paws-cough.mdpackages/blocks/product-details/src/frontend/ProductDetails.client.tsxpackages/blocks/recommended-products/src/frontend/RecommendedProducts.client.tsxpackages/ui/src/components/Carousel/Carousel.tsxpackages/ui/src/components/Carousel/Carousel.types.tspackages/ui/src/components/Carousel/useManagedCarouselKeyboard.tspackages/ui/src/components/ProductCarousel/ProductCarousel.tsxpackages/ui/src/components/ProductCarousel/ProductCarousel.types.tspackages/ui/src/components/ProductGallery/ProductGallery.tsxpackages/ui/src/components/ProductGallery/ProductGallery.types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui/src/components/ProductGallery/ProductGallery.types.ts
- .changeset/wet-paws-cough.md
- packages/ui/src/components/Carousel/Carousel.types.ts
marcinkrasowski
approved these changes
Mar 9, 2026
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
tomaszpacior
deleted the
684-bug-incorrect-keyboard-handling-on-product-details-page
branch
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
2 participants
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.
What does this PR do?
Related Ticket(s)
Key Changes
Introduced managed keyboard navigation for Swiper-based carousels so only one active carousel responds to
ArrowLeft/ArrowRightat a time.Added a shared active-carousel manager and reusable keyboard hook in
@o2s/ui:carouselKeyboardManager.tsuseManagedCarouselKeyboard.tsExtended carousel API with:
keyboardControlMode(swiper-native|managed|off)keyboardCarouselIdRemoved
defaultKeyboardActivefrom the API and implementation.Updated
Carousel,ProductGallery, andProductCarouselto use the managed keyboard flow.Added automatic
keyboardCarouselIdresolution inProductCarouselwhen not provided in props (manual wiring is no longer required in typical usage).Side effects:
swiper-nativemode is preserved (backward-compatible default inCarousel).How to test
Setup:
npm install).Test steps:
are visible at the same time.
ArrowRight/ArrowLeft.Validation run:
npm run build(repo root) ✅npm run lintin:packages/uipackages/blocks/product-detailspackages/blocks/recommended-productsSummary by CodeRabbit
New Features
Chores