788-feat: Add filter for merch #904
Conversation
antonina220590
requested review from
Quiddlee,
SpaNb4,
andron13,
ansivgit,
dzmitry-varabei and
natanchik
as code owners
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Lighthouse Report:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
src/widgets/merch-section/ui/merch-catalog/layouts/layouts.module.scss (2)
42-42: Use the$opacity-0design token for consistency.Line 51 correctly uses
$opacity-100, but line 42 uses a literal0. Apply the opacity variable consistently.Based on past review comments.
106-111: Complete the styling for the active filter indicator.The
::afterpseudo-element has no visual styling (no background, border, or color), making it invisible.Based on past review comments.
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx (1)
57-57: Unskip critical search and clear tests.Tests at lines 57, 71, 100, 130, and 146 are skipped but cover essential filter functionality. Enable and fix any failures.
Based on past review comments.
src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.tsx (1)
22-27: Addtype="button"to prevent form submission.Buttons without an explicit
typedefault totype="submit", which can trigger unintended form submissions.Based on past review comments.
src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.tsx (1)
24-43: Add explicit button types to prevent unintended form submits.Lines 25 and 33 render buttons without
type, so they default tosubmitand will trigger stray form submissions on mobile/tablet layouts. Addtype="button"to both controls.- {hasActiveFilters && ( - <button className={cx('button', 'secondary', 'active')} onClick={onClearFilters}> + {hasActiveFilters && ( + <button + type="button" + className={cx('button', 'secondary', 'active')} + onClick={onClearFilters} + > Clear </button> )} ... - <button + <button + type="button" className={cx('tablet-toggle-button', { expanded: areTagsExpanded })} onClick={onToggleTagsExpansion} aria-expanded={areTagsExpanded}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/shared/__tests__/constants.tsx(2 hunks)src/views/merch.tsx(1 hunks)src/widgets/merch-catalog/index.ts(0 hunks)src/widgets/merch-section/index.ts(1 hunks)src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.test.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/layouts/layouts.module.scss(1 hunks)src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-catalog.module.scss(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-catalog.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-filters/search-filters/search-filters.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filter.test.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.module.scss(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/types.ts(1 hunks)src/widgets/merch-section/ui/merch-section.module.scss(0 hunks)src/widgets/merch-section/ui/merch-section.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/widgets/merch-catalog/index.ts
- src/widgets/merch-section/ui/merch-section.module.scss
✅ Files skipped from review due to trivial changes (1)
- src/widgets/merch-section/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/tests/constants.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx (2)
src/shared/hooks/use-media-query/use-media-query.ts (1)
useMediaQuery(3-18)src/widgets/merch-section/ui/merch-catalog/merch-catalog.tsx (1)
MerchCatalog(19-132)
src/views/merch.tsx (6)
src/widgets/merch-section/ui/loader/loader.tsx (1)
Loader(1-3)src/widgets/merch-section/ui/merch-section.tsx (1)
MerchSection(13-38)src/widgets/merch-catalog/ui/merch-catalog.tsx (1)
products(13-34)src/widgets/merch-catalog/ui/merch-list/merch-list.tsx (3)
MerchListProps(21-72)page(48-54)product(59-61)src/entities/merch/model/store.ts (1)
MerchStore(5-21)src/widgets/merch-catalog/ui/loader/loader.tsx (1)
div(1-3)
src/widgets/merch-section/ui/merch-section.tsx (6)
src/shared/ui/paragraph/paragraph.tsx (1)
Paragraph(24-36)src/widgets/merch-section/ui/merch-catalog/merch-catalog.tsx (1)
MerchCatalog(19-132)src/widgets/merch-catalog/ui/merch-list/merch-list.tsx (3)
MerchListProps(21-72)page(48-54)product(59-61)src/entities/merch/ui/merch-card/merch-card.tsx (1)
MerchProduct(17-74)src/entities/merch/model/store.ts (1)
MerchStore(5-21)src/entities/merch/helpers/transform-merch-catalog.ts (2)
category(12-28)data(3-35)
src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.tsx (1)
src/widgets/merch-section/ui/merch-catalog/types.ts (1)
LayoutMobileProps(24-27)
src/widgets/merch-section/ui/merch-catalog/merch-filters/search-filters/search-filters.tsx (1)
src/shared/ui/search-input/search-input.tsx (1)
SearchInput(15-33)
src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filter.test.tsx (1)
src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.tsx (1)
TagFilters(13-51)
src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.tsx (2)
src/widgets/merch-section/ui/merch-catalog/types.ts (1)
LayoutProps(17-22)src/shared/ui/subtitle/subtitle.tsx (1)
Subtitle(36-59)
src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.test.tsx (2)
src/shared/__tests__/constants.tsx (1)
getMockedProps(232-243)src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.tsx (1)
DesktopMerchFilters(10-35)
src/widgets/merch-section/ui/merch-catalog/merch-catalog.tsx (8)
src/widgets/merch-section/ui/merch-catalog/types.ts (1)
MerchProductsProps(29-32)src/widgets/merch-section/ui/merch-catalog/merch-list/merch-list.tsx (1)
MerchList(21-72)src/shared/ui/paragraph/paragraph.tsx (1)
Paragraph(24-36)src/widgets/merch-section/ui/merch-catalog/merch-filters/search-filters/search-filters.tsx (1)
SearchFilters(8-18)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.tsx (1)
TagFilters(13-51)src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.tsx (1)
DesktopMerchFilters(10-35)src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.tsx (1)
MobileMerchFilters(11-49)src/widgets/merch-catalog/ui/merch-catalog.tsx (1)
products(13-34)
src/widgets/merch-section/ui/merch-catalog/types.ts (3)
src/widgets/merch-catalog/ui/merch-list/merch-list.tsx (2)
MerchListProps(21-72)product(59-61)src/entities/merch/ui/merch-card/merch-card.tsx (1)
MerchProduct(17-74)src/entities/merch/model/store.ts (1)
MerchStore(5-21)
🔇 Additional comments (4)
src/views/merch.tsx (1)
6-7: LGTM!The import and component rename from
MerchCatalogtoMerchSectionis consistent and correctly applied.Also applies to: 15-15
src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filter.test.tsx (1)
1-53: LGTM!Test coverage is thorough: rendering, empty state, checked state, and selected class styling. All tests are properly enabled.
src/widgets/merch-section/ui/merch-catalog/merch-filters/search-filters/search-filters.tsx (1)
1-18: LGTM!Clean wrapper component with proper prop wiring to
SearchInput.src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.module.scss (1)
1-87: LGTM!Tag filter styling is well-structured with proper hover, focus, and checked states. The visually-hidden checkbox pattern is correctly implemented.
.../merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.test.tsx
Outdated
Show resolved
Hide resolved
.../merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.test.tsx
Outdated
Show resolved
Hide resolved
src/widgets/merch-section/ui/merch-catalog/layouts/layouts.module.scss
Outdated
Show resolved
Hide resolved
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx
Outdated
Show resolved
Hide resolved
src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.tsx
Outdated
Show resolved
Hide resolved
| const allTags: string[] = products.flatMap((product) => product.tags).filter((tag) => tag.length); | ||
| const uniqueTags: string[] = Array.from(new Set(allTags)).sort(); |
There was a problem hiding this comment.
Guard against missing tags before flatMap filtering.
Line 16 assumes every product has tags; when a product arrives without tags the filter runs tag.length on undefined, crashing the server component. Please default to an empty array before flattening.
- const allTags: string[] = products.flatMap((product) => product.tags).filter((tag) => tag.length);
+ const allTags: string[] = products
+ .flatMap((product) => product.tags ?? [])
+ .filter((tag) => tag.length);📝 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.
| const allTags: string[] = products.flatMap((product) => product.tags).filter((tag) => tag.length); | |
| const uniqueTags: string[] = Array.from(new Set(allTags)).sort(); | |
| const allTags: string[] = products | |
| .flatMap((product) => product.tags ?? []) | |
| .filter((tag) => tag.length); | |
| const uniqueTags: string[] = Array.from(new Set(allTags)).sort(); |
🤖 Prompt for AI Agents
In src/widgets/merch-section/ui/merch-section.tsx around lines 16 to 17, the
code calls product.tags assuming it always exists which will throw if a
product's tags are undefined; update the flatten step to default missing tags to
an empty array (e.g., use product.tags ?? [] or product.tags || []) before
flatMap so you only call .filter on strings from real tag arrays, then continue
deduping and sorting as before.
|
Lighthouse Report:
|
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/widgets/merch-section/ui/merch-catalog/layouts/layouts.module.scss (2)
41-44: Use$opacity-0token for consistency.Line 42 still hardcodes
opacity: 0;. Swap in the design token to stay aligned with the rest of the palette.
105-110: Finish the active-filter dot styling.The pseudo-element is created but has no color, so the indicator never appears. Add the missing background (and radius if it should be circular) to make it visible or drop the rule altogether.
🧹 Nitpick comments (2)
src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.test.tsx (2)
8-8: Move userEvent setup into each test.
userEvent.setup()at the describe level shares state across tests. Per Testing Library best practices, callsetup()within each test or in abeforeEachto ensure test isolation.- const user = userEvent.setup(); - const mockSearchFilters = <div data-testid="search-filters">Mock Search</div>;Then in each test:
it('should render the "Clear" button only when hasActiveFilters is true', async () => { const user = userEvent.setup(); // ... rest of test });
98-100: querySelector tests implementation detail.Line 98 uses
querySelector('.filter-toggle-arrow')to test the arrow's rotation class. Since the arrow is decorative (alt=""), there's no accessible query. Consider whether testing arrow rotation is necessary, or accept this as a pragmatic visual behavior check.Alternatives:
- Skip testing arrow rotation; focus on accordion open/closed state (tag filters visibility).
- Add
data-testidto the arrow if visual rotation is critical to test.Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/widgets/merch-section/ui/merch-catalog/layouts/layouts.module.scss(1 hunks)src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.test.tsx (1)
src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.tsx (1)
MobileMerchFilters(11-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI
🔇 Additional comments (2)
src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.test.tsx (2)
13-27: LGTM!The test correctly verifies always-rendered elements using accessible queries.
29-61: LGTM!The test correctly verifies conditional rendering and interaction. Proper use of
rerenderandqueryByRolefor absence checks.
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx (3)
142-142: Use standard Vitest matchers.
toHaveBeenCalledExactlyOnceWithis not a standard matcher. This was previously flagged.Replace with:
- expect(mockRouterReplace).toHaveBeenCalledExactlyOnceWith('/merch?search=Test', { scroll: false }); + expect(mockRouterReplace).toHaveBeenCalledWith('/merch?search=Test', { scroll: false });Note: Line 141 already asserts
toHaveBeenCalledTimes(1), so the second assertion only needs to check arguments.
153-153: Use standard Vitest matchers.Same issue as line 142.
- expect(mockRouterReplace).toHaveBeenCalledExactlyOnceWith('/merch?type=clothing', { scroll: false }); + expect(mockRouterReplace).toHaveBeenCalledWith('/merch?type=clothing', { scroll: false });
165-165: Use standard Vitest matchers.Same issue as lines 142 and 153.
- expect(mockRouterReplace).toHaveBeenCalledExactlyOnceWith('/merch?type=unisex', { scroll: false }); + expect(mockRouterReplace).toHaveBeenCalledWith('/merch?type=unisex', { scroll: false });
🧹 Nitpick comments (2)
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx (2)
14-15: Minor formatting inconsistency.The type annotation spans lines awkwardly. Consider formatting on a single line or with clearer breaks.
-let mockSearchParams: { search?: string; - types?: string[]; } = {}; +let mockSearchParams: { search?: string; types?: string[] } = {};
74-176: Consider expanding test coverage.Current tests cover basic filtering and URL updates. Consider adding tests for:
- Multiple type filters simultaneously
- Combination of search + type filters
- The
onClearFilterscallback (no test currently verifies clearing resets URL)- Edge case: empty
initialProductsarray- Edge case: null/undefined
initialAvailableTags
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filter.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filter.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx (4)
src/widgets/merch-section/ui/merch-catalog/types.ts (3)
MerchProductsProps(29-32)LayoutProps(17-22)LayoutMobileProps(24-27)src/widgets/merch-section/ui/merch-catalog/merch-catalog.tsx (1)
MerchCatalog(19-132)src/widgets/merch-section/ui/merch-catalog/merch-filters/search-filters/search-filters.tsx (1)
SearchFilters(8-18)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.tsx (1)
TagFilters(13-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Create Preview
- GitHub Check: CI
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/widgets/merch-section/ui/merch-catalog/layouts/layouts.module.scss (2)
42-42: Use the$opacity-0design token for consistency.Line 51 correctly uses
$opacity-100, but line 42 uses a literal0. Apply the opacity variable consistently.Apply this diff:
- opacity: 0; + opacity: $opacity-0;Based on past review comments.
105-110: Complete the styling for the active filter indicator.The
::afterpseudo-element has no visual styling (no background, border, or color), making it invisible. Either complete the styling or remove the rule.If this is meant to be a visual indicator, add the missing styles:
&.has-active-filters::after { content: ''; display: block; width: 6px; height: 6px; + background-color: $color-yellow; + border-radius: 50%; }Or remove the rule entirely if the indicator is not needed.
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx (3)
147-147: Use standard Vitest matchers.
toHaveBeenCalledExactlyOnceWithis not a standard matcher. UsetoHaveBeenCalledTimes(1)withtoHaveBeenCalledWith.Apply this diff:
- expect(mockRouterReplace).toHaveBeenCalledExactlyOnceWith('/merch?search=Test', { scroll: false }); + expect(mockRouterReplace).toHaveBeenCalledTimes(1); + expect(mockRouterReplace).toHaveBeenCalledWith('/merch?search=Test', { scroll: false });
158-158: Use standard Vitest matchers.
toHaveBeenCalledExactlyOnceWithis not a standard matcher. UsetoHaveBeenCalledTimes(1)withtoHaveBeenCalledWith.Apply this diff:
- expect(mockRouterReplace).toHaveBeenCalledExactlyOnceWith('/merch?type=clothing', { scroll: false }); + expect(mockRouterReplace).toHaveBeenCalledTimes(1); + expect(mockRouterReplace).toHaveBeenCalledWith('/merch?type=clothing', { scroll: false });
170-170: Use standard Vitest matchers.
toHaveBeenCalledExactlyOnceWithis not a standard matcher. UsetoHaveBeenCalledTimes(1)withtoHaveBeenCalledWith.Apply this diff:
- expect(mockRouterReplace).toHaveBeenCalledExactlyOnceWith('/merch?type=unisex', { scroll: false }); + expect(mockRouterReplace).toHaveBeenCalledTimes(1); + expect(mockRouterReplace).toHaveBeenCalledWith('/merch?type=unisex', { scroll: false });
🧹 Nitpick comments (1)
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx (1)
14-15: Fix type definition formatting.The line break in the middle of the type definition reduces readability.
Apply this diff:
-let mockSearchParams: { search?: string; - types?: string[]; } = {}; +let mockSearchParams: { search?: string; types?: string[]; } = {};
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/layouts/layouts.module.scss(1 hunks)src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-catalog.module.scss(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-catalog.tsx(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.module.scss(1 hunks)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.module.scss
- src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.tsx
- src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.tsx
- src/widgets/merch-section/ui/merch-catalog/merch-catalog.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.tsx (2)
src/widgets/merch-section/ui/merch-catalog/types.ts (1)
LayoutProps(17-22)src/shared/ui/subtitle/subtitle.tsx (1)
Subtitle(36-59)
src/widgets/merch-section/ui/merch-catalog/merch-catalog.test.tsx (7)
src/widgets/merch-section/ui/merch-catalog/types.ts (3)
MerchProductsProps(29-32)LayoutProps(17-22)LayoutMobileProps(24-27)src/widgets/merch-section/ui/merch-catalog/layouts/desktop-merch-filters/desktop-merch-filters.tsx (1)
DesktopMerchFilters(10-36)src/widgets/merch-section/ui/merch-catalog/layouts/mobile-merch-filters/mobile-merch-filters.tsx (1)
MobileMerchFilters(11-49)src/widgets/merch-section/ui/merch-catalog/merch-catalog.tsx (1)
MerchCatalog(19-132)src/widgets/merch-section/ui/merch-catalog/merch-filters/search-filters/search-filters.tsx (1)
SearchFilters(8-18)src/widgets/merch-section/ui/merch-catalog/merch-filters/tag-filters/tag-filters.tsx (1)
TagFilters(13-46)src/widgets/merch-catalog/ui/merch-list/merch-list.tsx (1)
MerchListProps(21-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI
5 participants
What type of PR is this? (select all that apply)
Description
Add filter for merch page for desktop and mobile view
Add tests for filter component
Related Tickets & Documents
Screenshots, Recordings
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Summary by CodeRabbit
New Features
UI/Style
Accessibility
Refactor
Tests