[602] feat: Add case study pagination and navigation#154
Conversation
- Extract tag counting logic into TagCountService - Add NavigationService for previous/next case study navigation - Implement previous/next buttons on case study detail pages - Add back to all projects link with preserved filter state - Preserve query parameters across navigation links - Add CSS styling for navigation controls
📝 Walkthrough## Walkthrough
This update modularizes tag counting and navigation logic for the case studies page by introducing `TagCountService` and `NavigationService` classes. It updates lifecycle methods to fetch navigation data before rendering, restructures template layouts to add navigation and query-preserving links, and introduces new SCSS styles for the case study navigation component.
## Changes
| Files/Groups | Change Summary |
|-------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `website/modules/asset/ui/src/scss/_cases.scss` | Added new SCSS styles for a "Case Study Navigation" component, including container, item, and text styles with responsive and state-based modifiers. |
| `website/modules/asset/ui/src/scss/_animation.scss` | Removed the `@keyframes tagSlideIn` animation definition. |
| `website/modules/case-studies-page/index.js` | Refactored to use new `TagCountService` and `NavigationService` for tag counting and navigation logic; added `beforeShow` lifecycle method to fetch navigation data; updated methods to delegate to services. |
| `website/modules/case-studies-page/services/NavigationService.js` | Introduced `NavigationService` class with static methods for retrieving, indexing, and navigating between case studies. |
| `website/modules/case-studies-page/services/TagCountService.js` | Introduced `TagCountService` class with static methods for tag mapping, counting, fetching, and processing related to case studies tags. |
| `website/modules/case-studies-page/services/UrlService.js` | Introduced `UrlService` class to build URLs with query parameters and attach navigation and tag count data to requests. |
| `website/modules/case-studies-page/views/index.html` | Modified article card links to generate URLs dynamically via `UrlService` to preserve and append filter/search query parameters. |
| `website/modules/case-studies-page/views/show.html` | Restructured template to add navigation (prev/next) and back links with query preservation, reorganized layout sections, and clarified image and testimonial rendering. |
| `website/modules/@apostrophecms/shared-constants/ui/src/index.js` | Changed unquoted keys in `STANDARD_FORM_FIELD_NAMES` object to quoted string keys; no functional changes. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Server
participant TagCountService
participant NavigationService
participant Templates
User->>Server: Request case study page (with filters/search)
Server->>TagCountService: Fetch and process tag counts
Server->>NavigationService: Fetch navigation data (prev/next)
Server->>Templates: Render page with tag counts and navigation data
Templates->>User: Display page with navigation, filters, and preserved queriesPossibly related PRs
Suggested reviewers
|
IhorMasechko
changed the title
🔍 Vulnerabilities of
|
| digest | sha256:0ea6ccb10e17ee1b1701c4e1a2a4f9e96204d9c3658086474e0e0b873e377b2e |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 290 MB |
| packages | 976 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| 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: 5
🧹 Nitpick comments (4)
website/modules/case-studies-page/services/TagCountService.js (1)
10-79: Consider using plain functions instead of a static-only class.Since this class contains only static methods, consider converting it to a collection of plain functions or a namespace object for better JavaScript practices.
-class TagCountService { - static createTagMap(casesTags) { +const TagCountService = { + createTagMap(casesTags) { const tagMap = {}; casesTags.forEach((tag) => { tagMap[tag.aposDocId] = tag.slug; }); return tagMap; - } + }, - static countTagsOfType(tagIds, tagMap, countsForType) { + countTagsOfType(tagIds, tagMap, countsForType) { // ... rest of the method - } + }, // ... other methods -} +};This would also eliminate the
thisvs class name confusion in static methods.🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
website/modules/case-studies-page/services/NavigationService.js (1)
10-91: Consider using plain functions instead of a static-only class.Since this class contains only static methods, consider converting it to a collection of plain functions or a namespace object for better JavaScript practices.
-class NavigationService { - static async getAllCaseStudies(req, apos, applyFilters = false, pageModule = null) { +const NavigationService = { + async getAllCaseStudies(req, apos, applyFilters = false, pageModule = null) { // ... method implementation - } + }, - static findCurrentIndex(allCaseStudies, currentPiece) { + findCurrentIndex(allCaseStudies, currentPiece) { // ... method implementation - } + }, // ... other methods -} +};This approach is more idiomatic in JavaScript and eliminates the
thisconfusion in static methods.🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 85-85: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
website/modules/case-studies-page/views/show.html (1)
47-47: Maintain consistent text for navigation items.The disabled navigation text is inconsistent - "previous" vs "next project".
- <span class="cs_nav-text cs_nav-text--disabled">previous</span> + <span class="cs_nav-text cs_nav-text--disabled">previous project</span>Also applies to: 71-71
website/modules/case-studies-page/index.js (1)
40-44: Remove unnecessaryreqCopyvariable.The
reqCopy = reqpattern is unnecessary since JavaScript objects are passed by reference. You're already modifying the originalreqobject.- const reqCopy = req; - if (!reqCopy.data) { - reqCopy.data = {}; + if (!req.data) { + req.data = {}; } - reqCopy.data.tagCounts = tagCounts; + req.data.tagCounts = tagCounts;Apply the same simplification to all four occurrences in this file.
Also applies to: 48-57, 64-70, 74-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
website/modules/asset/ui/src/scss/_cases.scss(1 hunks)website/modules/case-studies-page/index.js(3 hunks)website/modules/case-studies-page/services/NavigationService.js(1 hunks)website/modules/case-studies-page/services/TagCountService.js(1 hunks)website/modules/case-studies-page/views/index.html(1 hunks)website/modules/case-studies-page/views/show.html(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/case-studies-page/services/TagCountService.js
[error] 9-77: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
website/modules/case-studies-page/services/NavigationService.js
[error] 9-89: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 78-78: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 85-85: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
website/modules/case-studies-page/views/show.html (1)
6-19:Details
❓ Verification inconclusive
Use proper URL encoding to prevent injection vulnerabilities.
The current implementation manually encodes brackets but doesn't properly encode the parameter values. This could lead to URL injection vulnerabilities if the query parameters contain special characters.
Consider using a proper URL encoding utility or filter. Check if Nunjucks or ApostropheCMS provides URL building helpers:
If no built-in utility exists, ensure all parameter values are properly encoded:
-{% set queryParams = queryParams.concat(['industry%5B' + loop.index0 + '%5D=' + industry]) %} +{% set queryParams = queryParams.concat(['industry%5B' + loop.index0 + '%5D=' + industry | urlencode]) %}Also applies to: 30-41, 54-65
🏁 Script executed:
#!/bin/bash # Search for URL encoding utilities in the codebase rg -A 5 "urlencode|encodeURI|URLSearchParams" --type js --type html # Check if ApostropheCMS provides URL building helpers ast-grep --pattern 'apos.$_.$_url'Length of output: 106
Ensure safe URL encoding for query parameters
The manual
%5B/%5Dencoding only handles brackets and doesn’t escape other special characters in the values, opening the door to injection issues. Please verify whether your Nunjucks/Apostrophe setup provides a filter likeurlencode(or a URL‐building helper). If not, add one or sanitize every dynamic segment.• File: website/modules/case-studies-page/views/show.html
– Affected blocks: lines 6–19 (also apply similar changes at lines 30–41 and 54–65)Example replacement:
-{% set queryParams = queryParams.concat(['industry%5B' + loop.index0 + '%5D=' + industry]) %} +{% set queryParams = queryParams.concat([ + 'industry[' ~ loop.index0 ~ ']=' ~ (industry | urlencode) +]) %}Repeat for
stack,caseStudyType, andsearch, ensuring every value is passed through the encoder.website/modules/case-studies-page/index.js (1)
2-3: Good refactoring to extract services!The extraction of tag counting and navigation logic into dedicated services improves modularity and testability. This follows the single responsibility principle well.
Also applies to: 95-103, 108-117
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
website/modules/case-studies-page/services/NavigationService.js (3)
37-37: Replacethiswith class name in static methods (additional instances).These lines have the same issue flagged in past reviews for lines 136-145. Using
thisin static context is confusing since it refers to the class constructor.Apply this diff to fix the remaining instances:
- const industryIds = await this.convertSlugsToIds( + const industryIds = await NavigationService.convertSlugsToIds( apos, req, req.query.industry, ); - const stackIds = await this.convertSlugsToIds(apos, req, req.query.stack); + const stackIds = await NavigationService.convertSlugsToIds(apos, req, req.query.stack); - const caseStudyTypeIds = await this.convertSlugsToIds( + const caseStudyTypeIds = await NavigationService.convertSlugsToIds( apos, req, req.query.caseStudyType, );Also applies to: 50-50, 57-57
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
89-89: Replacethiswith class name in static method.Same issue as flagged in past reviews - using
thisin static context is confusing.- query = await this.applyFiltersToQuery(query, req, apos); + query = await NavigationService.applyFiltersToQuery(query, req, apos);🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
135-146: Past review comment still applies - replacethiswith class name.The previous review comment about replacing
thiswithNavigationServicein static methods still needs to be addressed.🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 143-143: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 145-145: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🧹 Nitpick comments (1)
website/modules/case-studies-page/services/NavigationService.js (1)
10-147: Consider the trade-offs of static-only class vs. module functions.While static analysis suggests using simple functions instead of a static-only class, the current approach provides good namespace organization and follows common service patterns. However, you could alternatively export individual functions from this module if you prefer a more functional approach.
The current class-based approach is acceptable for a service layer in an enterprise application context.
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-147: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 57-57: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 89-89: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 136-136: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 143-143: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 145-145: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/case-studies-page/services/NavigationService.js(1 hunks)website/modules/case-studies-page/views/show.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/case-studies-page/views/show.html
🧰 Additional context used
🧬 Code Graph Analysis (1)
website/modules/case-studies-page/services/NavigationService.js (2)
website/modules/case-studies-page/index.js (1)
NavigationService(3-3)scripts/merged-prs-last-24h.js (1)
req(109-167)
🪛 Biome (1.9.4)
website/modules/case-studies-page/services/NavigationService.js
[error] 9-147: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 57-57: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 89-89: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 136-136: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 143-143: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 145-145: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: security-scan
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (5)
website/modules/case-studies-page/services/NavigationService.js (5)
1-10: Excellent documentation and clear service responsibilities.The class documentation clearly defines the single responsibility and provides a comprehensive overview of the service's capabilities.
18-24: Well-implemented parallel tag lookup with proper error handling.The method efficiently fetches tags in parallel and correctly filters out failed lookups before extracting document IDs.
80-93: Solid implementation of filtered case study fetching.The conditional filter application and sorting logic is well-implemented for the ApostropheCMS environment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
101-104: Clean and appropriate index lookup implementation.The method correctly finds the current case study index, and the eslint disable is justified for ApostropheCMS
_idfield access.
112-125: Excellent navigation calculation with proper boundary handling.The method correctly handles edge cases for first and last items in the navigation sequence.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/modules/case-studies-page/services/NavigationService.js (1)
43-43:⚠️ Potential issueReplace
thiswith class name in static methods.The static analysis correctly identifies that using
thisin static context is confusing since it refers to the class constructor. This is the same issue flagged in previous reviews.Apply this fix to improve clarity:
- const industryIds = await this.convertSlugsToIds( + const industryIds = await NavigationService.convertSlugsToIds( apos, req, req.query.industry, ); - const stackIds = await this.convertSlugsToIds(apos, req, req.query.stack); + const stackIds = await NavigationService.convertSlugsToIds(apos, req, req.query.stack); - const caseStudyTypeIds = await this.convertSlugsToIds( + const caseStudyTypeIds = await NavigationService.convertSlugsToIds( apos, req, req.query.caseStudyType, ); - query = await this.applyFiltersToQuery(query, req, apos); + query = await NavigationService.applyFiltersToQuery(query, req, apos); - const allCaseStudies = await this.getAllCaseStudies( + const allCaseStudies = await NavigationService.getAllCaseStudies( req, apos, true, pageModule, ); - const currentIndex = this.findCurrentIndex(allCaseStudies, currentPiece); + const currentIndex = NavigationService.findCurrentIndex(allCaseStudies, currentPiece); - return this.calculatePrevNext(allCaseStudies, currentIndex); + return NavigationService.calculatePrevNext(allCaseStudies, currentIndex);Also applies to: 56-56, 63-63, 95-95, 142-142, 149-149, 151-151
🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🧹 Nitpick comments (1)
website/modules/case-studies-page/services/NavigationService.js (1)
10-153: Consider refactoring to use simple functions instead of static-only class.While the current implementation works, a class containing only static methods could be refactored to use simple exported functions, which might be more idiomatic and easier to test.
Example approach:
// Convert class to module with exported functions const convertSlugsToIds = async (apos, req, slugs) => { // implementation }; const applyFiltersToQuery = async (query, req, apos) => { // implementation }; // ... other functions module.exports = { convertSlugsToIds, applyFiltersToQuery, getAllCaseStudies, findCurrentIndex, calculatePrevNext, getNavigationData };🧰 Tools
🪛 Biome (1.9.4)
[error] 9-153: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 56-56: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 63-63: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 95-95: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 142-142: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/case-studies-page/index.js(3 hunks)website/modules/case-studies-page/services/NavigationService.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/case-studies-page/services/NavigationService.js
[error] 9-153: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 56-56: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 63-63: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 95-95: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 142-142: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: security-scan
🔇 Additional comments (6)
website/modules/case-studies-page/index.js (4)
2-3: LGTM! Good modularization.The imports properly introduce the new service classes to handle tag counting and navigation logic, improving separation of concerns.
60-82: Well-structured lifecycle method with proper error handling.The
beforeShowmethod follows the same pattern asbeforeIndexwith consistent error handling and data structure initialization. The fallback values ensure the application remains functional even when navigation data cannot be retrieved.
95-103: Good refactoring to use TagCountService.The method now properly delegates to the external service, improving code organization and reusability.
108-117: Clean navigation data method.The method appropriately delegates to the NavigationService and provides all necessary parameters for navigation calculation.
website/modules/case-studies-page/services/NavigationService.js (2)
18-30: Solid slug-to-ID conversion logic.The method properly handles asynchronous tag lookups, filters out null results, and extracts the correct document IDs. The use of Promise.all is appropriate for parallel processing.
118-131: Clean boundary handling for navigation calculation.The method correctly handles edge cases for first and last items in the list, ensuring proper null values when no previous or next items exist.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
website/modules/case-studies-page/services/TagCountService.js (1)
66-76: Replacethiswith class name in static methods for clarity.This issue was already identified in previous reviews and still needs to be addressed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
website/modules/case-studies-page/services/NavigationService.js (1)
142-151: Replacethiswith class name in static methods for clarity.This issue was already identified in previous reviews and still needs to be addressed.
🧰 Tools
🪛 Biome (1.9.4)
[error] 142-142: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🧹 Nitpick comments (2)
website/modules/case-studies-page/services/TagCountService.js (1)
10-77: Consider using a module with exported functions instead of a static-only class.While the current class structure works, you could simplify this to a module that exports individual functions, which might be more idiomatic for this use case.
-class TagCountService { - static createTagMap(casesTags) { +function createTagMap(casesTags) { // ... implementation - } +} - static countTagsOfType(tagIds, tagMap, countsForType) { +function countTagsOfType(tagIds, tagMap, countsForType) { // ... implementation - } +} // ... other functions -module.exports = TagCountService; +module.exports = { + createTagMap, + countTagsOfType, + fetchCaseStudiesAndTags, + processCaseStudies +};🧰 Tools
🪛 Biome (1.9.4)
[error] 9-77: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
website/modules/case-studies-page/services/NavigationService.js (1)
10-153: Consider using a module with exported functions instead of a static-only class.Similar to TagCountService, you could simplify this to a module that exports individual functions for better consistency with functional programming patterns.
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-153: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 56-56: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 63-63: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 95-95: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 142-142: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/modules/case-studies-page/index.js(3 hunks)website/modules/case-studies-page/services/NavigationService.js(1 hunks)website/modules/case-studies-page/services/TagCountService.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/case-studies-page/index.js
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/case-studies-page/services/NavigationService.js
[error] 9-153: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 56-56: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 63-63: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 95-95: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 142-142: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
website/modules/case-studies-page/services/TagCountService.js
[error] 9-77: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: security-scan
- GitHub Check: unit-tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
website/modules/case-studies-page/services/TagCountService.js (3)
16-22: LGTM! Clean mapping implementation.The tag mapping logic is straightforward and correctly handles the transformation from tag objects to a lookup dictionary.
30-43: LGTM! Proper null safety and counting logic.The method correctly handles edge cases with the optional chaining and length check, and the counting logic is sound.
52-56: LGTM! Straightforward data fetching.The async data fetching logic is clean and returns the expected tuple of case studies and tags.
website/modules/case-studies-page/services/NavigationService.js (3)
18-30: LGTM! Efficient slug-to-ID conversion with proper null handling.The use of Promise.all for concurrent tag lookups and filtering of null results is well implemented.
107-110: LGTM! Efficient index finding with clear underscore dangle comment.The findIndex implementation is straightforward and the eslint disable comment appropriately documents the underscore-dangle exception.
118-131: LGTM! Correct navigation boundary logic.The previous/next calculation properly handles edge cases for first and last items in the list.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
website/modules/case-studies-page/services/TagCountService.js (2)
66-76: Replacethiswith class name in static methods for clarity.Using
thisin static context is confusing since it refers to the class constructor. Use the class name directly for better readability.Apply this diff to fix the static method calls:
static processCaseStudies(caseStudies, tagMap, tagCounts) { caseStudies.forEach((study) => { - this.countTagsOfType(study.industryIds, tagMap, tagCounts.industry); + TagCountService.countTagsOfType(study.industryIds, tagMap, tagCounts.industry); - this.countTagsOfType(study.stackIds, tagMap, tagCounts.stack); + TagCountService.countTagsOfType(study.stackIds, tagMap, tagCounts.stack); - this.countTagsOfType( + TagCountService.countTagsOfType( study.caseStudyTypeIds, tagMap, tagCounts.caseStudyType, ); }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
92-103: Replacethiswith class name in static methods for clarity.The same issue occurs in the
calculateTagCountsmethod wherethisis used to call other static methods.Apply this diff:
const [caseStudies, casesTags] = await this.fetchCaseStudiesAndTags( req, aposModules, options, ); - const tagMap = this.createTagMap(casesTags); + const tagMap = TagCountService.createTagMap(casesTags); - this.processCaseStudies(caseStudies, tagMap, tagCounts); + TagCountService.processCaseStudies(caseStudies, tagMap, tagCounts);🧰 Tools
🪛 Biome (1.9.4)
[error] 92-92: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 98-98: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 100-100: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
website/modules/case-studies-page/services/NavigationService.js (3)
43-47: Replacethiswith class name in static methods for clarity.Using
thisin static context is confusing since it refers to the class constructor. Use the class name directly for better readability.Apply this diff to fix the static method calls in
applyFiltersToQuery:if (req.query.industry && req.query.industry.length > 0) { - const industryIds = await this.convertSlugsToIds( + const industryIds = await NavigationService.convertSlugsToIds( apos, req, req.query.industry, ); // ... rest of method } if (req.query.stack && req.query.stack.length > 0) { - const stackIds = await this.convertSlugsToIds(apos, req, req.query.stack); + const stackIds = await NavigationService.convertSlugsToIds(apos, req, req.query.stack); // ... rest of method } if (req.query.caseStudyType && req.query.caseStudyType.length > 0) { - const caseStudyTypeIds = await this.convertSlugsToIds( + const caseStudyTypeIds = await NavigationService.convertSlugsToIds( apos, req, req.query.caseStudyType, ); // ... rest of method }Also applies to: 56-56, 63-67
🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
95-95: Replacethiswith class name in static methods for clarity.Using
thisin static context is confusing since it refers to the class constructor. Use the class name directly for better readability.Apply this diff:
- query = await this.applyFiltersToQuery(query, req, apos); + query = await NavigationService.applyFiltersToQuery(query, req, apos);🧰 Tools
🪛 Biome (1.9.4)
[error] 95-95: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
142-151: Replacethiswith class name in static methods for clarity.Using
thisin static context is confusing since it refers to the class constructor. Use the class name directly for better readability.Apply this diff to fix the static method calls in
getNavigationData:static async getNavigationData(req, apos, pageModule, currentPiece) { - const allCaseStudies = await this.getAllCaseStudies( + const allCaseStudies = await NavigationService.getAllCaseStudies( req, apos, true, pageModule, ); - const currentIndex = this.findCurrentIndex(allCaseStudies, currentPiece); + const currentIndex = NavigationService.findCurrentIndex(allCaseStudies, currentPiece); - return this.calculatePrevNext(allCaseStudies, currentIndex); + return NavigationService.calculatePrevNext(allCaseStudies, currentIndex); }🧰 Tools
🪛 Biome (1.9.4)
[error] 142-142: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🧹 Nitpick comments (1)
website/modules/case-studies-page/services/TagCountService.js (1)
10-104: Consider using individual functions instead of a static-only class.While the current class structure provides good organization, consider whether individual exported functions might be more appropriate since this class contains only static members.
Alternative approach:
-class TagCountService { - static createTagMap(casesTags) { +function createTagMap(casesTags) { // implementation - } +} - static countTagsOfType(tagIds, tagMap, countsForType) { +function countTagsOfType(tagIds, tagMap, countsForType) { // implementation - } +} // Export as an object +module.exports = { + createTagMap, + countTagsOfType, + fetchCaseStudiesAndTags, + processCaseStudies, + calculateTagCounts, +};🧰 Tools
🪛 Biome (1.9.4)
[error] 9-104: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 92-92: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 98-98: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 100-100: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 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/services/NavigationService.js(1 hunks)website/modules/case-studies-page/services/TagCountService.js(1 hunks)website/modules/case-studies-page/services/UrlService.js(1 hunks)website/modules/case-studies-page/views/index.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/case-studies-page/views/index.html
🧰 Additional context used
🧬 Code Graph Analysis (1)
website/modules/case-studies-page/services/TagCountService.js (2)
website/modules/case-studies-page/index.js (1)
TagCountService(2-2)scripts/merged-prs-last-24h.js (1)
req(109-167)
🪛 Biome (1.9.4)
website/modules/case-studies-page/services/TagCountService.js
[error] 9-104: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 66-66: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 92-92: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 98-98: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 100-100: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
website/modules/case-studies-page/services/UrlService.js
[error] 9-98: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 81-81: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
website/modules/case-studies-page/services/NavigationService.js
[error] 9-166: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 56-56: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 63-63: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 95-95: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 142-142: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 149-149: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 164-164: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: security-scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
website/modules/case-studies-page/services/UrlService.js (1)
17-55: URL building logic looks solid.The implementation correctly handles query parameters with proper encoding and supports both single values and arrays. The use of URLSearchParams ensures proper URL encoding.
🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 38-38: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
website/modules/case-studies-page/index.js (3)
2-4: Good separation of concerns with new service imports.The extraction of tag counting, navigation, and URL building logic into dedicated services improves code organization and maintainability.
48-64: Excellent error handling in setupIndexData.The method properly handles potential failures and provides sensible fallback data, ensuring the application doesn't break if tag counting fails.
66-78: Consistent error handling pattern in setupShowData.The error handling follows the same pattern as setupIndexData with appropriate fallbacks for navigation data.
website/modules/case-studies-page/services/NavigationService.js (2)
18-30: Robust slug-to-ID conversion logic.The implementation correctly handles parallel lookups and filters out null results. Good error handling for missing tags.
118-131: Clean navigation calculation logic.The previous/next calculation correctly handles edge cases at the beginning and end of the list.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
website/modules/case-studies-page/services/UrlService.js (1)
10-98: Consider functional module pattern over static-only class.Similar to
TagCountService, this static-only class could be better implemented as a module with exported functions, which is more idiomatic in Node.js environments.🧰 Tools
🪛 Biome (1.9.4)
[error] 9-98: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
website/modules/case-studies-page/services/NavigationService.js (1)
10-178: Excellent modularization but consider functional approach.The service effectively centralizes navigation logic with good separation of concerns. However, like the other services, this static-only class could benefit from a functional module pattern for better JavaScript idioms.
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-178: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🧹 Nitpick comments (3)
website/modules/case-studies-page/services/TagCountService.js (1)
10-105: Consider using a module with exported functions instead of a static-only class.While the current implementation works, a static-only class goes against JavaScript conventions. Consider refactoring to a module with exported functions for better alignment with Node.js patterns.
Example structure:
// TagCountService.js function createTagMap(casesTags) { // current logic } function countTagsOfType(tagIds, tagMap, countsForType) { // current logic } // ... other functions module.exports = { createTagMap, countTagsOfType, fetchCaseStudiesAndTags, processCaseStudies, calculateTagCounts };🧰 Tools
🪛 Biome (1.9.4)
[error] 9-105: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
website/modules/case-studies-page/services/UrlService.js (1)
74-82: Request object mutation pattern could be improved.While functional, directly mutating the request object (
reqCopy = req) doesn't actually create a copy. Consider making this more explicit about the mutation or using a different pattern.static attachIndexData(req, tagCounts) { - const reqCopy = req; - if (!reqCopy.data) { - reqCopy.data = {}; + if (!req.data) { + req.data = {}; } - reqCopy.data.tagCounts = tagCounts; - reqCopy.data.buildCaseStudyUrl = (caseStudyUrl) => + req.data.tagCounts = tagCounts; + req.data.buildCaseStudyUrl = (caseStudyUrl) => UrlService.buildCaseStudyUrl(caseStudyUrl, req.query); }website/modules/case-studies-page/services/NavigationService.js (1)
111-114: Consider using more descriptive variable names.The ESLint disable comment suggests awareness of the underscore-prefixed
_idfield. Consider adding a comment explaining why this is necessary (ApostropheCMS convention).static findCurrentIndex(allCaseStudies, currentPiece) { - // eslint-disable-next-line no-underscore-dangle + // ApostropheCMS uses _id as the document identifier + // eslint-disable-next-line no-underscore-dangle return allCaseStudies.findIndex((study) => study._id === currentPiece._id); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/modules/case-studies-page/services/NavigationService.js(1 hunks)website/modules/case-studies-page/services/TagCountService.js(1 hunks)website/modules/case-studies-page/services/UrlService.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
website/modules/case-studies-page/services/NavigationService.js (2)
website/modules/case-studies-page/index.js (1)
NavigationService(3-3)scripts/merged-prs-last-24h.js (1)
req(109-167)
website/modules/case-studies-page/services/TagCountService.js (2)
website/modules/case-studies-page/index.js (1)
TagCountService(2-2)scripts/merged-prs-last-24h.js (1)
req(109-167)
🪛 Biome (1.9.4)
website/modules/case-studies-page/services/NavigationService.js
[error] 9-178: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
website/modules/case-studies-page/services/TagCountService.js
[error] 9-105: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
website/modules/case-studies-page/services/UrlService.js
[error] 9-98: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e-tests
- GitHub Check: security-scan
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
website/modules/case-studies-page/services/TagCountService.js (2)
1-22: Well-structured tag mapping logic.The
createTagMapmethod correctly creates a mapping from tag IDs to slugs, handling the data transformation cleanly.
30-43: Robust tag counting with proper validation.Good defensive programming with the optional chaining check
!tagIds?.lengthand proper validation before incrementing counts.website/modules/case-studies-page/services/UrlService.js (2)
17-55: Excellent URL building with proper parameter handling.The implementation correctly:
- Uses
URLSearchParamsfor proper encoding- Handles both single values and arrays consistently
- Preserves all relevant query parameters (industry, stack, caseStudyType, search)
- Returns clean URLs with or without query strings
62-67: Simple and effective array normalization.The
ensureArrayutility correctly handles both array and non-array inputs, providing consistent behavior for query parameter processing.website/modules/case-studies-page/services/NavigationService.js (2)
18-30: Solid slug-to-ID conversion with proper filtering.The implementation correctly:
- Uses
Promise.allfor concurrent tag lookups- Filters out null results from failed lookups
- Maps to the expected
aposDocIdfield
39-80: Comprehensive filter application logic.The method effectively handles multiple filter types (industry, stack, caseStudyType) and correctly applies MongoDB
$inqueries with proper conditional logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
website/modules/case-studies-page/views/show.html (4)
47-54: Addrel="prev"for SEO-friendly pagination
Annotating the “previous project” anchor withrel="prev"helps search engines understand the page sequence.-<a href="{{ prevUrl }}" class="cs_nav-link cs_nav-link--active" aria-label="Navigate to previous project"> +<a href="{{ prevUrl }}" rel="prev" class="cs_nav-link cs_nav-link--active" aria-label="Navigate to previous project">
64-79: Improve disabled state semantics with ARIA
Assistive technologies will correctly announce this as non-interactive if you includearia-disabled="true".-<span class="cs_nav-link cs_nav-link--disabled" aria-label="Previous project (unavailable)"> +<span class="cs_nav-link cs_nav-link--disabled" aria-label="Previous project (unavailable)" aria-disabled="true">
83-90: Addrel="next"for SEO-friendly pagination
Similar to the previous link, userel="next"on the “next project” anchor to signal the logical sequence to crawlers.-<a href="{{ nextUrl }}" class="cs_nav-link cs_nav-link--active" aria-label="Navigate to next project"> +<a href="{{ nextUrl }}" rel="next" class="cs_nav-link cs_nav-link--active" aria-label="Navigate to next project">
100-115: Improve disabled state semantics with ARIA
For consistency, mark the disabled “next project” span witharia-disabled="true".-<span class="cs_nav-link cs_nav-link--disabled" aria-label="Next project (unavailable)"> +<span class="cs_nav-link cs_nav-link--disabled" aria-label="Next project (unavailable)" aria-disabled="true">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
website/public/images/arrow-back-desktop.svgis excluded by!**/*.svgwebsite/public/images/arrow-left.svgis excluded by!**/*.svgwebsite/public/images/arrow-right.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
website/modules/asset/ui/src/scss/_cases.scss(1 hunks)website/modules/case-studies-page/views/show.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/modules/asset/ui/src/scss/_cases.scss
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: security-scan
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
website/modules/case-studies-page/views/show.html (1)
18-21: Leverage the new macro on the back link
UsingbuildQueryString(data.query)here cleanly replaces the previous inline logic and correctly preserves filter state on the “Back to all projects” link.
IhorMasechko
dismissed stale reviews from killev, yuramax, VitalyyP, Anton-88, and coderabbitai[bot]
via
3e98932
|
5 participants
This feature enhances user experience by allowing seamless navigation between case studies while preserving filter context, making it easier for users to browse through projects without losing their current search or filter state.