[595] implement category display in sidebar for case studies#106
[595] implement category display in sidebar for case studies#106IhorMasechko merged 10 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThe changes refactor the case studies filtering and display system to use three distinct filter categories—industry, stack (technology), and caseStudyType—replacing a previous single tag filter. Data structures, templates, and admin UI are updated to support multi-valued fields and categorized tag relationships, with new helper methods for tag counting and display. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CaseStudiesPage
participant CaseStudiesModule
participant CasesTagsModule
participant Database
User->>CaseStudiesPage: Load page with filter(s)
CaseStudiesPage->>CaseStudiesModule: Fetch case studies with filters (industry, stack, caseStudyType)
CaseStudiesPage->>CasesTagsModule: Fetch tag metadata
CaseStudiesPage->>CaseStudiesModule: Calculate tag counts (beforeIndex hook)
CaseStudiesModule->>Database: Query case studies and tags
Database-->>CaseStudiesModule: Return data
CaseStudiesModule-->>CaseStudiesPage: Return case studies and tag counts
CaseStudiesPage-->>User: Render page with filtered studies, tag counts, and filter UI
Possibly related PRs
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
website/modules/case-studies/index.js (1)
179-198: Optimize the category accessor with optional chainingThe helper function correctly groups tags by category, but the nested property check could be simplified using optional chaining.
- const category = - (tag._category && tag._category[0] && tag._category[0].title) || - 'Uncategorized'; + const category = tag._category?.[0]?.title || 'Uncategorized';Additionally, consider adding a check for the existence of the
labelproperty before pushing it to the array to prevent potential undefined values:- grouped[category].push(tag.label); + if (tag.label) { + grouped[category].push(tag.label); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 188-188: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
website/modules/case-studies-page/views/index.html (1)
74-77: Consider optimizing tag matching logicThe current implementation requires additional iteration to find the full tag object that matches each label. This works but could be more efficient.
Consider modifying the
groupTagsByCategoryhelper to retain the full tag objects instead of just the labels. This would eliminate the need for the nested loop to find matching tags:// In case-studies/index.js: groupTagsByCategory(tags) { const grouped = {}; if (!Array.isArray(tags)) { return grouped; } tags.forEach((tag) => { const category = tag._category?.[0]?.title || 'Uncategorized'; if (!grouped[category]) { grouped[category] = []; } grouped[category].push(tag); // Store the full tag object }); return grouped; }Then in the template:
-{% for label in tagLabels %} - {# Find the tag object that matches this label #} - {% set matchingTag = null %} - {% for tag in tags %} - {% if tag.label == label %} - {% set matchingTag = tag %} - {% endif %} - {% endfor %} - {% if matchingTag %} +{% for tag in tagObjects %}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/modules/case-studies-page/views/index.html(1 hunks)website/modules/case-studies/index.js(3 hunks)website/modules/cases-tags/index.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/case-studies/index.js
[error] 188-188: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: security-scan
- GitHub Check: unit-tests
🔇 Additional comments (5)
website/modules/case-studies/index.js (2)
12-12: Good addition of the alias!Adding the
alias: 'caseStudy'creates a convenient way to access this piece type from templates, which I can see is used in the case studies page template.
85-85: Properly extending tag relationship to include categoriesThe inclusion of
_categoryin both the projection andwithRelationshipsensures that category data is available when retrieving tags, which is essential for the new grouping functionality.Also applies to: 88-88
website/modules/cases-tags/index.js (1)
7-7:Details
✅ Verification successful
Keyboard shortcut updated
The keyboard shortcut has been changed from "G,M" to "G,H". Ensure this doesn't conflict with other shortcuts in the system.
🏁 Script executed:
#!/bin/bash # Check for any other components using the G,H shortcut rg "shortcut:\s*['\"](G,H|H,G)['\"]" --type jsLength of output: 109
Shortcut “G,H” Conflict Check Passed
A search across JS files found no other occurrences of the
shortcut: 'G,H'binding—only inwebsite/modules/cases-tags/index.js. No conflicts detected. Proceed as is.website/modules/case-studies-page/views/index.html (2)
70-72: Good implementation of categorized tag displayUsing the new helper function to group tags by category creates a more organized UI structure. The template now displays category headers, which improves navigation for users.
78-96: Tag filtering logic preserved correctlyThe logic for active state identification and URL building for tag filtering has been maintained while incorporating the new category structure. This ensures users can still filter tags as before, now with the added benefit of categorical organization.
🔍 Vulnerabilities of
|
| digest | sha256:53e9cbebd7fcca2c6951e1c39342040d0c8cf52641408ff5e39f1df4ffabddae |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 284 MB |
| packages | 935 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:169ee1b69bd3f5c7c8508a9919e8b367bba9dc7fa9a03097ec9f85d2e8179631 |
| vulnerabilities |  |
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/modules/case-studies/index.js (1)
179-199: Well-implemented helper function with room for improvement.The
groupTagsByCategoryhelper function correctly organizes tags by their category titles, with proper handling of edge cases like non-array inputs and tags without categories. However, the category determination logic could be more concise.Consider using optional chaining for more concise code:
- const category = - (tag._category && tag._category[0] && tag._category[0].title) || - 'Uncategorized'; + const category = tag._category?.[0]?.title || 'Uncategorized';This makes the code more readable while maintaining the same functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/modules/case-studies/index.js(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/case-studies/index.js
[error] 189-189: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
website/modules/case-studies/index.js (2)
12-12: Good addition of the module alias.Adding the alias
caseStudymakes the module methods accessible from templates viaapos.caseStudy, which is essential for calling the new helper function from the template as mentioned in the PR summary.
85-85: Properly configured tag-category relationship.The addition of
_category: 1to the projection andwithRelationships: ['_category']ensures that when tags are fetched, their associated category data is included and populated. This is essential for the grouping functionality to work correctly.Also applies to: 88-88
- 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
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
website/modules/case-studies-page/views/show.html (1)
55-61: Apply the same improvements to_caseStudyTypeblockThe same
joinoptimisation and null-safety comment for_industryapplies here. Keeping the two sections consistent reduces maintenance friction.
🧹 Nitpick comments (10)
website/modules/case-studies-page/views/show.html (2)
42-49: Preferjoinfilter over manual comma logicInstead of looping and adding a conditional comma, you can lean on Nunjucks’ built-in
joinfilter:-{% for industry in data.piece._industry %} - {{ industry.title }}{% if not loop.last %}, {% endif %} -{% endfor %} +{{ data.piece._industry | map(attribute='title') | join(', ') }}This shortens the template, eliminates the repeated
{% if not loop.last %}pattern, and avoids the risk of a trailing comma should the loop logic ever change.
97-104:_stackrendering duplicates earlier patternsThree nearly identical code blocks (
industry,type,stack) repeat the same join/guard logic. Consider extracting a small macro:{% macro renderList(items) %} {{ items | map(attribute='title') | join(', ') }} {% endmacro %}Then call
{{ _self.renderList(data.piece._stack) }}etc. This keeps the template DRY.website/modules/case-studies-page/views/index.html (1)
91-153: Repeated filter-type branches violate DRYEach branch (
industry,stack,caseStudyType) duplicates identical HTML except for the query-key. A small macro can collapse ~60 lines:{% macro tagLink(filter, verb, tag, count) -%} <a class="tag-link" href="{{ data.url | build({}, { [filter]: { [verb]: tag.value } }) }}"> {{ tag.label }} [{{ count }}] </a> {%- endmacro %}Using
tagLink(filterType, '$addToSet', tag, count)etc. will simplify maintenance and reduce merge conflicts.website/app.test.js (1)
9-19: Avoid re-assigningprocess.env; restore values insteadReassigning the whole
process.envobject (process.env = originalEnv) is discouraged in Node.js because other modules may hold references to the original object.
A safer pattern is to mutate keys that were changed during the test and delete the ones you added:afterEach(() => { - process.env = originalEnv; + // Restore mutated keys + Object.keys(process.env).forEach((k) => { + if (!(k in originalEnv)) { + delete process.env[k]; + } + }); + Object.assign(process.env, originalEnv); });This keeps the original object instance intact while still rolling back the state.
website/modules/case-studies/index.js (1)
41-67: Minor: browser projection omitsslug, which is often usefulIf the UI needs the slug for links, consider adding
slug: 1to bothbuilders.projectandbrowser.projectionsections of_stack,_industry, and_caseStudyType.
This avoids a follow-up round-trip if slugs become necessary.website/modules/case-studies-page/index.js (1)
69-108: Performance consideration: cache tag counts
beforeIndexfetches all case studies and tags on every page request, which can be expensive once the collection grows.Consider caching the computed
tagCounts(e.g., in Redis orself.apos.cache) and invalidating onafterInsert,afterUpdate, andafterDeleteof case studies or tags.docs/Infrastructure.md (4)
8-8: Consistent hyphenation for ‘multi-environment’
Consider using a single compound word “multienvironment” or removing the hyphen in line with your style guide to avoid inconsistency.🧰 Tools
🪛 LanguageTool
[misspelling] ~8-~8: This word is normally spelled as one.
Context: ...tructure**: Modular Terraform setup for multi-environment support * Resource Tags (applied to...(EN_COMPOUNDS_MULTI_ENVIRONMENT)
69-69: Add hyphen for ‘container-specific’
Update “container specific permissions” to “container-specific permissions” for grammatical correctness.🧰 Tools
🪛 LanguageTool
[uncategorized] ~69-~69: When ‘container-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...: Grants the application running in the container specific permissions to AWS services * **Resou...(SPECIFIC_HYPHEN)
85-85: Add missing preposition in ECS Task Execution Role
To improve clarity, consider:-* Grants permissions to pull container images and send logs +* Grants permissions to pull container images and to send logsOr specify the destination: “send logs to CloudWatch.”
🧰 Tools
🪛 LanguageTool
[uncategorized] ~85-~85: Possible missing preposition found.
Context: ...ll container images and send logs * Resource Tags: * `Name: sf-website-ecs-exe...(AI_HYDRA_LEO_MISSING_TO)
231-231: Remove redundant “only” in TLS line
For brevity, change:-* Modern TLS protocol versions only +* Modern TLS protocol versions🧰 Tools
🪛 LanguageTool
[style] ~231-~231: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...only * Modern TLS protocol versions only * Security headers added via respon...(ADVERB_REPETITION_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docker-compose.yml(1 hunks)docs/Infrastructure.md(1 hunks)scripts/merged-prs-last-24h.js(1 hunks)website/.eslintrc.js(1 hunks)website/app.js(2 hunks)website/app.test.js(1 hunks)website/e2e/tests/screenshot-test.spec.js(1 hunks)website/modules/@apostrophecms/express/index.js(1 hunks)website/modules/@apostrophecms/form/index.js(2 hunks)website/modules/@apostrophecms/uploadfs/index.js(1 hunks)website/modules/asset/index.js(1 hunks)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(6 hunks)website/modules/cases-tags/index.js(3 hunks)website/modules/map-widget/views/widget.html(1 hunks)website/utils/env.js(1 hunks)website/utils/env.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- scripts/merged-prs-last-24h.js
- website/modules/@apostrophecms/form/index.js
- website/modules/map-widget/views/widget.html
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/cases-tags/index.js
🧰 Additional context used
🧬 Code Graph Analysis (3)
website/modules/@apostrophecms/express/index.js (3)
website/app.js (1)
require(3-3)website/modules/@apostrophecms/uploadfs/index.js (1)
require(1-1)website/utils/env.js (1)
getEnv(2-8)
website/e2e/tests/screenshot-test.spec.js (1)
website/utils/env.js (1)
getEnv(2-8)
website/modules/asset/index.js (1)
website/utils/env.js (1)
getEnv(2-8)
🪛 Biome (1.9.4)
website/modules/case-studies/index.js
[error] 253-253: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
docs/Infrastructure.md
[misspelling] ~8-~8: This word is normally spelled as one.
Context: ...tructure**: Modular Terraform setup for multi-environment support * Resource Tags (applied to...
(EN_COMPOUNDS_MULTI_ENVIRONMENT)
[uncategorized] ~69-~69: When ‘container-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...: Grants the application running in the container specific permissions to AWS services * **Resou...
(SPECIFIC_HYPHEN)
[uncategorized] ~85-~85: Possible missing preposition found.
Context: ...ll container images and send logs * Resource Tags: * `Name: sf-website-ecs-exe...
(AI_HYDRA_LEO_MISSING_TO)
[style] ~231-~231: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...only * Modern TLS protocol versions only * Security headers added via respon...
(ADVERB_REPETITION_PREMIUM)
🔇 Additional comments (30)
website/.eslintrc.js (1)
83-83: LGTM: ESLint rule updated to support loop variablesThe ESLint rule for
id-lengthhas been modified to allow short variable names (_,i,j) as exceptions. This change aligns with the PR objectives that introduce tag grouping logic, which likely uses these variable names in loops and iterators.website/utils/env.js (1)
1-12: LGTM: Well-implemented environment variable utilityThis new utility properly centralizes environment variable access with clear error handling for undefined variables. The implementation follows best practices by ensuring required environment variables are present and providing descriptive error messages when they're missing.
website/modules/@apostrophecms/express/index.js (2)
1-2: LGTM: Proper import of environment utilityThe import statement correctly brings in the
getEnvutility with an appropriate relative path.
7-7: LGTM: Enhanced environment variable access for session secretReplacing direct access to
process.env.SESSION_SECRETwithgetEnv('SESSION_SECRET')improves error handling by ensuring the session secret is defined, which is critical for security.website/modules/asset/index.js (2)
1-1: LGTM: Proper import of environment utilityThe import statement correctly brings in the
getEnvutility with an appropriate relative path.
8-8: LGTM: Enhanced environment variable access for NODE_ENVReplacing direct access to
process.env.NODE_ENVwithgetEnv('NODE_ENV')improves error handling by ensuring the environment variable is defined, which is important for proper environment-specific behavior.website/e2e/tests/screenshot-test.spec.js (2)
2-2: Good approach centralizing environment variable accessImporting the new
getEnvutility function follows best practices by centralizing environment variable access, which improves error handling throughout the application.
10-10:Details
✅ Verification successful
Verify BASE_URL environment variable is properly set
The change from direct
process.env.BASE_URLaccess togetEnv('BASE_URL')removes any implicit fallback behavior. Make sure this variable is properly set in all environments where tests run.
🏁 Script executed:
#!/bin/bash # Description: Verify BASE_URL is defined in relevant environment files echo "Checking for BASE_URL in configuration files:" rg "BASE_URL" -A 1 -B 1 --glob "!*.js" --glob "!*.md" --glob "!*.json" --glob "!*.lock" echo -e "\nChecking if other e2e tests might be affected by this change:" rg "process\.env\.BASE_URL" --glob "*.js" --glob "*.jsx" --glob "*.ts" --glob "*.tsx"Length of output: 955
Environment variable BASE_URL is properly configured
I’ve verified that:
BASE_URLis set in docker-compose.yml (http://localhost:3000) and docker-compose.e2e.yml (http://apostrophe:3000).- Playwright’s config falls back to
'http://localhost:3000'whenprocess.env.BASE_URLis unset.website/app.test.jsexplicitly assignsprocess.env.BASE_URLfor its tests.No further changes are needed to ensure
getEnv('BASE_URL')will always receive a value in existing environments.website/utils/env.test.js (1)
1-33: Well-structured tests for the env utilityThe test file is well-organized with proper test isolation using Jest lifecycle hooks. It thoroughly covers both success and error cases for the
getEnvfunction.A few observations:
- Excellent use of
beforeEachandafterAllto preserve and restore the original environment- Clear test descriptions that explain the expected behavior
- Good test structure with arrange/act/assert pattern
- Complete coverage of both successful retrieval and error handling scenarios
website/app.js (3)
3-3: Good approach importing the centralized env utilityImporting the
getEnvutility is a positive step toward standardizing environment variable access throughout the application.
8-8: Environment variable validation improvedReplacing direct
process.envaccess withgetEnv()calls ensures that missing configuration will fail fast with clear error messages rather than causing subtle issues later.Also applies to: 17-17, 21-21
28-35: Well-implemented Nunjucks integration for env variablesMaking the
getEnvfunction available to templates is an excellent approach that allows consistent environment variable access across both server-side code and templates.This ensures the same validation and error handling behavior for environment variables accessed from templates.
docker-compose.yml (4)
40-41: Redis and BASE_URL configuration properly addedThe addition of REDIS_URI and BASE_URL environment variables aligns with the refactoring to use
getEnv()throughout the codebase.
45-54: AWS S3 configuration improved and documentedThe S3 settings are now better organized with helpful comments and additional configuration options like APOS_CDN_URL, APOS_S3_STYLE, and APOS_S3_HTTPS.
55-57: Google Cloud Storage configuration addedThe addition of Google Cloud Storage settings expands the storage options available to the application.
Note that storing the private key directly in environment variables should be done with caution, especially if using non-secret environment variables elsewhere. Consider using a secret management solution for production environments.
62-62: Redis dependency correctly addedAdding Redis to the
depends_onlist ensures that the Redis service is started before the Apostrophe service, which is necessary since the application now requires Redis.website/modules/case-studies-page/views/show.html (1)
43-44: Guard against_industrybeing undefined
data.piece._industry.lengthassumes the array is defined. Although Nunjucks is forgiving, an explicit guard improves readability and prevents surprises if the field is evernull:{% if data.piece._industry and data.piece._industry.length %}(or use
|lengthwhich will coerce safely).website/modules/@apostrophecms/uploadfs/index.js (1)
17-20: Fail-fast when CDN URL is absent
cdn.enabledis hard-codedtrue, yetgetEnv('APOS_CDN_URL')will throw if the variable is missing. Confirm that is desired for local development; otherwise wrap with a conditional or default.website/modules/case-studies-page/index.js (2)
120-133: Possible mismatch in join-ID property namesThe code expects
industryIds,stackIds, andcaseStudyTypeIdson each study.
Apostrophe creates<fieldName>Idsby stripping the leading underscore from the join field, so for_industrythe property isindustryIds– this is correct. 👍
5-9:Details
❓ Verification inconclusive
aposDocIdis not a guaranteed field – prefer_idThe helper stores tags by
tag.aposDocId, but Apostrophe pieces expose their primary key as_id.
Unless you have custom code addingaposDocId, the lookup will fail andtagMapwill be empty, producing zero counts.- tagMap[tag.aposDocId] = tag.slug; + tagMap[tag._id] = tag.slug;Please confirm which identifier is actually present in your tag docs.
I’ll verify whether
aposDocIdactually exists on your tag docs and inspect how tags are defined. Continuing with a few repository searches:
🏁 Script executed:
#!/bin/bash # Search for any occurrences of aposDocId in the codebase echo ">>> Searching for aposDocId references" rg -n "aposDocId" # Locate the createTagMap implementation echo -e "\n>>> Locating createTagMap" rg -n "createTagMap" # Find where casesTags is sourced or iterated echo -e "\n>>> Locating casesTags references" rg -n "casesTags" # Discover all apostrophe-piece modules to find the tags schema echo -e "\n>>> Listing all apostrophe-pieces modules" rg -n "extend: 'apostrophe-pieces'" website/modulesLength of output: 15275
Verify correct identifier for tag mapping
It looks like you’re using
tag.aposDocIdto key the slug map, but I didn’t find any definition of anaposDocIdfield on yourcases-tagspiece schema. Apostrophe pieces expose their primary key as_idby default, so ifaposDocIdisn’t actually present on each tag doc this lookup will silently fail and your map will be empty.• File:
website/modules/case-studies-page/index.js(around line 8)Suggested change:
- tagMap[tag.aposDocId] = tag.slug; + tagMap[tag._id] = tag.slug;Please confirm whether your tag docs include an
aposDocIdproperty at runtime or if you should switch to using_id.docs/Infrastructure.md (10)
1-2: Documentation added: Final Terraform Infrastructure Plan
Great addition—provides a clear high-level overview of the infrastructure components.
3-16: General section is well-structured
Covers regions, environments, domain naming, and tagging conventions concisely.🧰 Tools
🪛 LanguageTool
[misspelling] ~8-~8: This word is normally spelled as one.
Context: ...tructure**: Modular Terraform setup for multi-environment support * Resource Tags (applied to...(EN_COMPOUNDS_MULTI_ENVIRONMENT)
19-41: Attachments Bucket details are comprehensive
Lifecycle rules, versioning, encryption, and CORS are all covered.
42-62: Logs Bucket configuration looks solid
Retention and transition policies are clearly defined.
98-114: S3 Bucket Access Policy is detailed and secure
Scoped permissions and resource tagging are clearly defined.
117-143: Amazon ECR section is thorough
Lifecycle policy and scan-on-push settings align with best practices.
146-184: Amazon ECS (Fargate) configuration is complete
Resource allocation, networking, and environment variable definitions are well-documented.
185-202: ALB configuration is clear
HTTPS-only enforcement and ACM integration are correctly specified.
236-257: CloudWatch monitoring and alerts are robust
Log retention and Slack integrations cover operational requirements.
260-298: MongoDB EC2 deployment section is comprehensive
Future HA plan and backup strategy are well-outlined.
- 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
website/modules/case-studies/index.js (1)
197-211:⚠️ Potential issueBug in
groupTagsByCategoryfunctionThe function attempts to push
tag.labelto the grouped array, but based on the field projections (lines 48, 63, 78), you're only projectingtitle, notlabel. This will result in undefined values being added to the category groups.- grouped[category].push(tag.label); + grouped[category].push(tag.title);website/modules/case-studies-page/index.js (1)
104-127: Clean implementation of tag counting methodsThe calculateTagCounts method is well-structured and uses the helper functions effectively. It initializes the tag counts object, fetches the necessary data, and processes it to calculate the counts.
The previous issue with calling an undefined
logFirstCaseStudyfunction has been fixed, as noted in past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/modules/case-studies-page/index.js(2 hunks)website/modules/case-studies-page/views/index.html(4 hunks)website/modules/case-studies/index.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/case-studies-page/views/index.html
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: security-scan
🔇 Additional comments (9)
website/modules/case-studies/index.js (7)
12-12: Alias added for programmatic accessAdding an alias for the case study module allows for direct programmatic access via
apos.caseStudyin other parts of the codebase, which is good practice for important core modules.
41-55: Field type change from string to relationship enhances data structureConverting the stack field from a string type to a relationship type with proper projections and relationship configuration improves data structure and enables the categorization feature required by this PR.
56-70: Field type change for case study type enhances categorizationSimilar to the stack field, changing the case study type to a relationship field with proper projections allows for better categorization and filtering.
71-85: Industry field upgrade to relationship typeConverting industry to a relationship field completes the set of categorizable fields, maintaining consistency across all tag types.
156-159: Group configuration updated to reflect new field namesThe group configuration has been correctly updated to reference the renamed relationship fields.
176-179: Column configuration updated for relationship fieldThe column configuration has been updated to use the new
_stackrelationship field name, maintaining the same component for display.
184-192: Filters updated to reflect new relationship fieldsFilter configuration has been updated to use the three distinct filter categories (industry, case study type, and stack) instead of a generic tags filter, improving the user filtering experience.
website/modules/case-studies-page/index.js (2)
55-61: Filter structure improved with dedicated category filtersReplacing the generic tags filter with specific filters for industry, stack, and case study type aligns with the PR objective of implementing category-based filtering. The additional options for pieces and piecesFiltersUrl provide proper configuration for the filters.
78-102: Error handling in tag counting initializationThe init method with the beforeIndex hook provides a robust way to calculate tag counts before rendering the page. The error handling ensures that even if tag counting fails, the page will still render with empty tag counts rather than breaking completely.
yuramax
left a comment
yuramax
left a comment
There was a problem hiding this comment.
Amazing work, @IhorMasechko! 👍
IhorMasechko
dismissed stale reviews from killev, Anton-88, VitalyyP, yuramax, and coderabbitai[bot]
via
13f5a0d
|
5 participants
This PR implements a new categorization system for case study tags, improving the organization and user experience of the case studies page.
Changes:
groupTagsByCategoryto organize tags by their categoriesTechnical Details:
_categoryrelationship to case study tagsTesting: