[594] Add the ability to search case studies tags#114
Conversation
- Refactor case-studies-page/index.js to fix ESLint errors while maintaining functionality - Split large functions into smaller utility functions - Fix race condition warnings using defensive copies - Implement dynamic tag counting with proper modularization - Update template to use the dynamic tag counts
- Remove invalid tagCountsMethods property from module top level - Move tag counting logic into helper utility functions - Simplify module structure to comply with ApostropheCMS conventions - Fix linter errors related to function length
- Remove main search form from case studies page - Add per-tag-type search input for filtering tags - Implement client-side tag filtering with JavaScript
📝 WalkthroughWalkthroughThe changes refactor the case studies page to support a multi-filter system with separate categories for industry, stack, and caseStudyType. The UI and template logic are updated to display and manage multiple values per category, and new JavaScript modules are added to enable client-side tag filtering within each category. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CaseStudiesPage (HTML/JS)
participant FilterLogic
participant Data
User->>CaseStudiesPage (HTML/JS): Selects filter (industry/stack/caseStudyType) or types in search
CaseStudiesPage (HTML/JS)->>FilterLogic: Updates filter state and triggers UI update
FilterLogic->>Data: Filters articles based on selected categories
Data-->>FilterLogic: Returns filtered article list
FilterLogic-->>CaseStudiesPage (HTML/JS): Renders updated articles and filter UI
User->>CaseStudiesPage (HTML/JS): Types in tag search input
CaseStudiesPage (HTML/JS)->>searchInputHandler.js: Filters visible tags in category section
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🔍 Vulnerabilities of
|
| digest | sha256:4e57dfeb98781f6dcd220e5d7678f6a9485fe4919a97d526445bad98a28810eb |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 284 MB |
| packages | 935 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:169ee1b69bd3f5c7c8508a9919e8b367bba9dc7fa9a03097ec9f85d2e8179631 |
| vulnerabilities |  |
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
website/modules/case-studies-page/index.js (1)
34-46: Consider using explicit object reference instead ofthisfor clarity.While the current implementation works, using
thisin object methods can lead to context issues if these methods are extracted or used as callbacks. Consider using the explicit object name for better maintainability.processCaseStudies(caseStudies, tagMap, tagCounts) { caseStudies.forEach((study) => { - this.countTagsOfType(study.industryIds, tagMap, tagCounts.industry); + tagCountHelpers.countTagsOfType(study.industryIds, tagMap, tagCounts.industry); - this.countTagsOfType(study.stackIds, tagMap, tagCounts.stack); + tagCountHelpers.countTagsOfType(study.stackIds, tagMap, tagCounts.stack); - this.countTagsOfType( + tagCountHelpers.countTagsOfType( study.caseStudyTypeIds, tagMap, tagCounts.caseStudyType, ); }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
website/modules/case-studies-page/index.js(2 hunks)website/modules/case-studies-page/views/index.html(4 hunks)website/modules/case-studies-page/views/show.html(2 hunks)website/modules/case-studies/index.js(4 hunks)website/modules/cases-tags/index.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (7)
website/modules/cases-tags/index.js (3)
25-25: LGTM! Consistent with relationship field requirements.Adding
_category: 1to the project builder is necessary for the tag categorization feature and aligns with the relationship field definitions.
47-53: LGTM! Category filtering support added correctly.The addition of category-based filtering enhances the admin interface capabilities and follows ApostropheCMS conventions.
7-7:Details
✅ Verification successful
Verify shortcut key uniqueness across the application.
The shortcut was changed from "G,M" to "G,H". Ensure this doesn't conflict with other modules or browser shortcuts.
🏁 Script executed:
#!/bin/bash # Check if the shortcut "G,H" is used elsewhere in the codebase rg -r '$1' 'shortcut:\s*["\']([^"\']*G,H[^"\']*)["\']' --type jsLength of output: 247
🏁 Script executed:
#!/bin/bash # Search all shortcut definitions in JS files to spot potential conflicts rg -n "shortcut:" --type js # Specifically look for any usage of "G,H" in JavaScript files rg -n "G,H" --type jsLength of output: 228
Shortcut ‘G,H’ uniqueness confirmed
- Searched all JS modules for
shortcut::
- website/modules/cases-tags/index.js (line 7):
‘G,H’- website/modules/team-members/index.js (line 9):
‘G,K’- No other occurrences of
‘G,H’found in the codebase.This change poses no cross-module shortcut conflicts. You may still want to verify that
G,Hisn’t reserved by your shortcut library or browser.website/modules/case-studies-page/views/index.html (1)
8-9: LGTM! Improved filter condition logic.The new condition properly checks for multiple filter types instead of a single generic filter, which aligns with the enhanced filtering capabilities.
website/modules/case-studies/index.js (1)
12-12: LGTM! Alias addition improves module accessibility.Adding the
caseStudyalias provides a more intuitive way to reference this piece type in code and queries.website/modules/case-studies-page/index.js (2)
55-61: Module configuration looks good!The configuration correctly defines the three distinct piece filters (industry, stack, caseStudyType) and properly sets the pieces source and filter URL. This aligns well with the refactoring from generic tags to relationship-based fields.
104-127: Clean implementation of tag counting logic!The
calculateTagCountsmethod is well-structured with proper initialization and effective use of helper functions. The separation of concerns makes the code easy to understand and maintain.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
website/modules/case-studies-page/views/index.html (3)
33-68: Reduce duplication in selected-tags rendering via a macro
The nested loops and repeatedif/eliffor each filter type have been flagged before. Extracting this into a macro (e.g.renderSelectedTag(filterType, tag, data)) will DRY up the logic and make future changes simpler.
76-152: DRY up filter sections with a macro
All three filter categories share nearly identical markup and logic. ArenderFilterSection(filterType, filterLabel, tags, data)macro (as suggested in earlier feedback) would consolidate this code, reducing maintenance overhead.
181-189:⚠️ Potential issueAdd defensive checks for
_industryand_stackrelationships
You’ve already guarded_caseStudyTypeagainst missing data, but the_industryand_stacksections assume those arrays always exist. If those properties are undefined, calling.lengthwill throw. Please update to:{% if article._industry and article._industry.length %} {# … #} {% endif %} … {% if article._stack and article._stack.length %} {# … #} {% endif %}Also applies to: 196-200
🧹 Nitpick comments (4)
website/modules/case-studies-page/views/index.html (4)
1-3: Template filename inconsistency: remove.newsuffix
The header comment referencesindex.new.html, but Apostrophe’s view discovery expectsindex.html. Please rename this file (and update any imports) so the template loader picks it up correctly.
8-9: Improve Jinja tag readability: separatesetandifstatements
The current combined tag makes scanning and diffs harder. Splitting them enhances clarity:{% set hasActiveFilters = data.query.industry or data.query.stack or data.query.caseStudyType %} {% if hasActiveFilters %} … {% endif %}
83-88: Extract inline styles to a CSS class
Avoid hard-codedstyle="margin-bottom: 10px; width: 100%"on the search input. Instead, define a utility class (e.g..filter-search-input) in your stylesheet and apply it here for consistent styling and easier theming.
214-237: Move inline JavaScript to an external file and add error handling
Embedding this script in the template prevents caching and mixes concerns. Consider:
- Extracting it to
assets/js/case-studies-filters.js.- Adding guards or optional chaining to prevent runtime errors:
document.addEventListener('DOMContentLoaded', () => { const inputs = document.querySelectorAll('.tag-search') || []; inputs.forEach(input => { input.addEventListener('input', () => { const items = input.closest('.filter-section')?.querySelectorAll('.tag-item') || []; items.forEach(item => { /* … */ }); }); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/case-studies-page/views/index.html(4 hunks)website/modules/case-studies-page/views/show.html(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/case-studies-page/views/show.html
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: security-scan
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
website/modules/case-studies-page/views/index.html (2)
187-189:⚠️ Potential issueAdd defensive check for industry relationship array.
The code assumes
article._industryis always an array, but there's no defensive check like the one implemented for_caseStudyType. This could cause runtime errors if the data is malformed.Apply this diff to add the defensive check:
- {% if article._industry.length %} {% for industry in + {% if article._industry and article._industry.length %} {% for industry in article._industry %} {{ industry.title }}{% if not loop.last %}, {% endif %} {% endfor %} {% endif %}
197-199:⚠️ Potential issueAdd defensive check for stack relationship array.
Similar to the industry section, the stack section lacks a defensive check to ensure
article._stackexists before accessing its length property.Apply this diff to add the defensive check:
- {% if article._stack.length %} {% for tech in article._stack %} {{ + {% if article._stack and article._stack.length %} {% for tech in article._stack %} {{ tech.title }}{% if not loop.last %}, {% endif %} {% endfor %} {% endif %}
🧹 Nitpick comments (1)
website/modules/case-studies-page/views/index.html (1)
183-184: Consider XSS protection for dynamic content.While the
titlefield is likely safe, consider adding explicit escaping for consistency and security best practices, especially since this data comes from relationship fields.Apply this diff for additional security:
- {% for type in article._caseStudyType %} {{ type.title | e }}{% if + {% for type in article._caseStudyType %} {{ type.title | e }}{% ifThe escaping is already present - this is good! Apply similar escaping to industry and stack sections:
{% if article._industry and article._industry.length %} {% for industry in - article._industry %} {{ industry.title }}{% if not loop.last %}, + article._industry %} {{ industry.title | e }}{% if not loop.last %},{% if article._stack and article._stack.length %} {% for tech in article._stack %} {{ - tech.title }}{% if not loop.last %}, {% endif %} {% endfor %} {% + tech.title | e }}{% if not loop.last %}, {% endif %} {% endfor %} {%
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/asset/ui/src/index.js(1 hunks)website/modules/case-studies-page/views/index.html(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
website/modules/asset/ui/src/index.js (2)
website/modules/asset/ui/src/swipers.js (1)
initAllSwipers(57-69)website/modules/asset/ui/src/smoothCounters.js (1)
initSmoothCounters(58-78)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: security-scan
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (1)
website/modules/asset/ui/src/index.js (1)
75-75: Confirm proper integration with page lifecycle.The
bindTagSearchfunction is correctly integrated into the initialization flow, ensuring it runs both on initial page load and after Barba.js page transitions.
IhorMasechko
dismissed stale reviews from VitalyyP and coderabbitai[bot]
via
122e5e0
VitalyyP
left a comment
VitalyyP
left a comment
There was a problem hiding this comment.
LGTM. Only notice - fix the PR description
Anton-88
left a comment
Anton-88
left a comment
There was a problem hiding this comment.
If there is no necessity in using inline styles, please, move them to class="tag-search" for your input
IhorMasechko
dismissed stale reviews from yuramax, VitalyyP, and Anton-88
via
ccf584a
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
website/modules/asset/ui/src/searchInputHandler.js (2)
26-38: Consider adding input validation.The function should validate that required parameters are provided before proceeding.
const handleTagSearch = ( input, containerSelector, tagSelector, getTagLabel, ) => { + if (!input || !containerSelector || !tagSelector || !getTagLabel) { + console.warn('Missing required parameters for tag search'); + return; + } + const filterValue = getFilterValue(input); const container = input.closest(containerSelector); if (!container) return; const tagItems = getTagItems(container, tagSelector); filterTagItems(tagItems, filterValue, getTagLabel); };
50-63: Consider performance optimization for large datasets.For better performance with large tag lists, consider adding debouncing to prevent excessive filtering on rapid input changes.
+const debounce = (func, wait) => { + let timeout; + return function executedFunction(...args) { + const later = () => { + clearTimeout(timeout); + func(...args); + }; + clearTimeout(timeout); + timeout = setTimeout(later, wait); + }; +}; + export const setupTagSearchForInput = ( input, { containerSelector, tagSelector, getTagLabel }, ) => { removePreviousHandler(input); - const handler = createTagSearchHandler( + const baseHandler = createTagSearchHandler( input, containerSelector, tagSelector, getTagLabel, ); + const handler = debounce(baseHandler, 150); input.tagSearchHandler = handler; input.addEventListener('input', handler); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/asset/ui/src/index.js(2 hunks)website/modules/asset/ui/src/searchInputHandler.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/asset/ui/src/index.js
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: security-scan
🔇 Additional comments (1)
website/modules/asset/ui/src/searchInputHandler.js (1)
1-5: LGTM! Good cleanup pattern for event handlers.The function properly removes previous event listeners to prevent memory leaks and duplicate handlers.
killev
left a comment
killev
left a comment
There was a problem hiding this comment.
This is not what I mean, but OK.
|
5 participants
This PR is needed to provide more flexible filtering for case studies, improve content relationships.