moved case studies filtering to frontend#272
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThis PR removes the server-side search/tag-count indexing pipeline from the case-studies-page module ( Sequence Diagram(s)Included above in the hidden review stack artifact. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@website/modules/case-studies-page/index.js`:
- Around line 103-112: The case studies index is loading the entire pieces
collection on every render, which can grow unbounded. Update the logic around
self.pieces.find(req).toArray() in the case-studies page handler to fetch only
the fields needed by the cards and data-search, and avoid projecting unused
data. Keep the Promise.all flow for casesTags and businessPartners, but cap the
piece payload so req.data.pieces, totalPieces, and totalPages are based on a
minimal result set. If full corpus loading is still intended, make that
expectation explicit and ensure the dataset is guaranteed to remain small.
In `@website/public/js/modules/case-studies-page/search-handler.js`:
- Around line 192-199: The selected-tag markup in search-handler.js is being
built with innerHTML, which leaves the label and data attributes as an injection
sink. Update the code in the selected-tag creation path to use DOM APIs instead
of a template string: create the li, text node, anchor, and image elements
explicitly, set their textContent and attribute values via setAttribute, and
append them together. Use the existing selected-tag/remove-tag logic and the
label/filterType/value variables as the identifiers to locate and refactor this
block.
- Around line 206-213: The visibility update in the search state handling only
toggles is-hidden, so .items-count__mobile never gets the class it needs to
become visible. Update the logic in the search-handler flow where
selectedTagsContainer, itemsCount, itemsCountMobile, and clearAll are toggled so
the mobile count also receives the matching is-visible state (or adjust the
related SCSS contract consistently). Use the existing itemsCountMobile symbol in
that block to keep the desktop and mobile visibility behavior aligned.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dea37748-4992-492b-883e-51fdf73fb2fd
📒 Files selected for processing (4)
website/modules/asset/ui/src/scss/_cases.scsswebsite/modules/case-studies-page/index.jswebsite/modules/case-studies-page/views/index.htmlwebsite/public/js/modules/case-studies-page/search-handler.js
| const [pieces, casesTags, businessPartners] = await Promise.all([ | ||
| self.pieces.find(req).toArray(), | ||
| self.apos.modules['cases-tags'].find(req).toArray(), | ||
| self.apos.modules['business-partner'].find(req).toArray(), | ||
| ]); | ||
| req.data.pieces = pieces; | ||
| req.data.totalPieces = pieces.length; | ||
| req.data.totalPages = 1; | ||
| req.data.casesTags = casesTags; | ||
| req.data.businessPartners = businessPartners; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Unbounded load of every case study on each index render.
self.pieces.find(req).toArray() pulls the full corpus (with all projected fields) on every request to hydrate client-side filtering. As the number of case studies grows this increases DB load, memory, and rendered page size linearly with no ceiling. Consider capping the projection to only the fields the cards/data-search actually need, and confirm the total volume is expected to stay small.
🤖 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 `@website/modules/case-studies-page/index.js` around lines 103 - 112, The case
studies index is loading the entire pieces collection on every render, which can
grow unbounded. Update the logic around self.pieces.find(req).toArray() in the
case-studies page handler to fetch only the fields needed by the cards and
data-search, and avoid projecting unused data. Keep the Promise.all flow for
casesTags and businessPartners, but cap the piece payload so req.data.pieces,
totalPieces, and totalPages are based on a minimal result set. If full corpus
loading is still intended, make that expectation explicit and ensure the dataset
is guaranteed to remain small.
| const li = document.createElement('li'); | ||
| li.className = 'selected-tag'; | ||
| li.innerHTML = ` | ||
| ${label} | ||
| <a class="remove-tag" href="javascript:void(0)" data-filter-type="${filterType}" data-tag-value="${value}" aria-label="Remove ${filterType} tag ${label}"> | ||
| <img src="/images/close.svg" alt="Close Icon"> | ||
| </a> | ||
| `; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Building selected-tag markup via innerHTML is an injection sink.
label (from data-tag-label, i.e. the tag title) and the interpolated attribute values are written into innerHTML unescaped. Any markup or quote in a tag title/value breaks out of the template and can inject script (stored XSS via editorial content). Build the node with createElement/textContent instead of a template string.
🔒️ Proposed fix using DOM APIs
- const li = document.createElement('li');
- li.className = 'selected-tag';
- li.innerHTML = `
- ${label}
- <a class="remove-tag" href="javascript:void(0)" data-filter-type="${filterType}" data-tag-value="${value}" aria-label="Remove ${filterType} tag ${label}">
- <img src="/images/close.svg" alt="Close Icon">
- </a>
- `;
- selectedTagsList.appendChild(li);
+ const li = document.createElement('li');
+ li.className = 'selected-tag';
+ li.appendChild(document.createTextNode(`${label} `));
+ const removeLink = document.createElement('a');
+ removeLink.className = 'remove-tag';
+ removeLink.href = 'javascript:void(0)';
+ removeLink.dataset.filterType = filterType;
+ removeLink.dataset.tagValue = value;
+ removeLink.setAttribute('aria-label', `Remove ${filterType} tag ${label}`);
+ const img = document.createElement('img');
+ img.src = '/images/close.svg';
+ img.alt = 'Close Icon';
+ removeLink.appendChild(img);
+ li.appendChild(removeLink);
+ selectedTagsList.appendChild(li);📝 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 li = document.createElement('li'); | |
| li.className = 'selected-tag'; | |
| li.innerHTML = ` | |
| ${label} | |
| <a class="remove-tag" href="javascript:void(0)" data-filter-type="${filterType}" data-tag-value="${value}" aria-label="Remove ${filterType} tag ${label}"> | |
| <img src="/images/close.svg" alt="Close Icon"> | |
| </a> | |
| `; | |
| const li = document.createElement('li'); | |
| li.className = 'selected-tag'; | |
| li.appendChild(document.createTextNode(`${label} `)); | |
| const removeLink = document.createElement('a'); | |
| removeLink.className = 'remove-tag'; | |
| removeLink.href = 'javascript:void(0)'; | |
| removeLink.dataset.filterType = filterType; | |
| removeLink.dataset.tagValue = value; | |
| removeLink.setAttribute('aria-label', `Remove ${filterType} tag ${label}`); | |
| const img = document.createElement('img'); | |
| img.src = '/images/close.svg'; | |
| img.alt = 'Close Icon'; | |
| removeLink.appendChild(img); | |
| li.appendChild(removeLink); | |
| selectedTagsList.appendChild(li); |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 193-198: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: li.innerHTML = ${label} <a class="remove-tag" href="javascript:void(0)" data-filter-type="${filterType}" data-tag-value="${value}" aria-label="Remove ${filterType} tag ${label}"> <img src="/images/close.svg" alt="Close Icon"> </a>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
🪛 OpenGrep (1.23.0)
[WARNING] 194-199: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
🤖 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 `@website/public/js/modules/case-studies-page/search-handler.js` around lines
192 - 199, The selected-tag markup in search-handler.js is being built with
innerHTML, which leaves the label and data attributes as an injection sink.
Update the code in the selected-tag creation path to use DOM APIs instead of a
template string: create the li, text node, anchor, and image elements
explicitly, set their textContent and attribute values via setAttribute, and
append them together. Use the existing selected-tag/remove-tag logic and the
label/filterType/value variables as the identifiers to locate and refactor this
block.
Source: Linters/SAST tools
| const itemsCount = document.querySelector('.items-count'); | ||
| const itemsCountMobile = document.querySelector('.items-count__mobile'); | ||
| const clearAll = document.querySelector('.clear-all'); | ||
|
|
||
| [selectedTagsContainer, itemsCount, itemsCountMobile, clearAll].forEach((el) => { | ||
| if (!el) return; | ||
| el.classList.toggle('is-hidden', !hasActiveFilters); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm nothing else toggles is-visible on items-count__mobile
rg -nP "items-count__mobile|is-visible" website/public/js website/modules -g '!**/node_modules/**'Repository: speedandfunction/website
Length of output: 1165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search-handler around the reported lines ==\n'
sed -n '180,225p' website/public/js/modules/case-studies-page/search-handler.js
printf '\n== template snippet for items-count__mobile ==\n'
sed -n '70,95p' website/modules/case-studies-page/views/index.html
printf '\n== SCSS snippet for items-count__mobile ==\n'
sed -n '580,605p' website/modules/asset/ui/src/scss/_cases.scss
printf '\n== filterModal usage of is-visible ==\n'
sed -n '60,125p' website/modules/asset/ui/src/filterModal.jsRepository: speedandfunction/website
Length of output: 5706
Toggle the mobile visibility class here too. website/public/js/modules/case-studies-page/search-handler.js:206-213 only flips is-hidden, but .items-count__mobile stays display: none unless is-visible is added. Add the matching class toggle here or change the SCSS contract.
🤖 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 `@website/public/js/modules/case-studies-page/search-handler.js` around lines
206 - 213, The visibility update in the search state handling only toggles
is-hidden, so .items-count__mobile never gets the class it needs to become
visible. Update the logic in the search-handler flow where
selectedTagsContainer, itemsCount, itemsCountMobile, and clearAll are toggled so
the mobile count also receives the matching is-visible state (or adjust the
related SCSS contract consistently). Use the existing itemsCountMobile symbol in
that block to keep the desktop and mobile visibility behavior aligned.
3 participants
Moved case study filtering from server-side URL navigation to a client-side experience. The listing page now preloads case-study, tag, and partner data, while the frontend search handler manages live keyword and tag filtering in memory, updates counts and empty states, and supports add/remove/clear actions without page redirects or infinite-scroll markup.