Use static navigation #3126
Use static navigation #3126
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
WalkthroughThe codebase has undergone a significant transformation, shifting from dynamic content retrieval to a static site structure. This change involved removing dynamic data fetching methods and properties like excerpts and KPIs. Instead, a static site navigation system was integrated, providing a more organized and stable content structure. This transition aims to streamline content delivery and simplify the site's navigation mechanism. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (5)
- baker/siteRenderers.tsx (3 hunks)
- db/wpdb.ts (4 hunks)
- packages/@ourworldindata/types/src/domainTypes/ContentGraph.ts (1 hunks)
- site/SiteNavigation.tsx (1 hunks)
- site/SiteNavigationTopics.tsx (1 hunks)
Additional comments: 6
packages/@ourworldindata/types/src/domainTypes/ContentGraph.ts (1)
- 3-17: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-11]
The removal of
excerptandkpifromEntryMetaand the change to makesubcategoriesoptional inCategoryWithEntriesare consistent with the PR objectives and summary.site/SiteNavigationTopics.tsx (1)
- 99-101: The use of the nullish coalescing operator to provide an empty array as a fallback for
category.subcategoriesis a good defensive programming practice.baker/siteRenderers.tsx (2)
- 207-207: The modification of
renderMenuJsonto returnJSON.stringify(SiteNavigationStatic)aligns with the PR's goal of transitioning to a static navigation structure.- 207-207: Verify that the static navigation object
SiteNavigationStaticis correctly structured and that all references to the old dynamic fetching method have been updated or removed.db/wpdb.ts (2)
- 52-52: Import of
SiteNavigationStaticis added to replace dynamic fetching with static navigation.- 331-342: The
isPostCitablefunction has been refactored to use the static navigation object instead of the removed dynamic fetching function.
| const entries = SiteNavigationStatic.categories | ||
| return entries.some((category) => { | ||
| return ( | ||
| category.entries.some((entry) => entry.slug === post.slug) || | ||
| category.subcategories.some((subcategory: CategoryWithEntries) => { | ||
| return subcategory.entries.some( | ||
| (subCategoryEntry) => subCategoryEntry.slug === post.slug | ||
| ) | ||
| }) | ||
| (category.subcategories ?? []).some( | ||
| (subcategory: CategoryWithEntries) => { | ||
| return subcategory.entries.some( | ||
| (subCategoryEntry) => | ||
| subCategoryEntry.slug === post.slug | ||
| ) | ||
| } | ||
| ) |
There was a problem hiding this comment.
The logic within isPostCitable assumes that the subcategories property always exists, which may not be the case according to the changes in ContentGraph.ts. Consider adding a nullish coalescing operator or default value for subcategories.
- (category.subcategories ?? []).some(
+ (category.subcategories || []).some(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.
| const entries = SiteNavigationStatic.categories | |
| return entries.some((category) => { | |
| return ( | |
| category.entries.some((entry) => entry.slug === post.slug) || | |
| category.subcategories.some((subcategory: CategoryWithEntries) => { | |
| return subcategory.entries.some( | |
| (subCategoryEntry) => subCategoryEntry.slug === post.slug | |
| ) | |
| }) | |
| (category.subcategories ?? []).some( | |
| (subcategory: CategoryWithEntries) => { | |
| return subcategory.entries.some( | |
| (subCategoryEntry) => | |
| subCategoryEntry.slug === post.slug | |
| ) | |
| } | |
| ) | |
| const entries = SiteNavigationStatic.categories | |
| return entries.some((category) => { | |
| return ( | |
| category.entries.some((entry) => entry.slug === post.slug) || | |
| (category.subcategories || []).some( | |
| (subcategory: CategoryWithEntries) => { | |
| return subcategory.entries.some( | |
| (subCategoryEntry) => | |
| subCategoryEntry.slug === post.slug | |
| ) | |
| } | |
| ) |
The isPostCitable function uses a deeply nested structure with multiple some and every calls. This could be refactored for better readability and potentially performance by flattening the structure or using a different approach to check for the post's citability.
There was a problem hiding this comment.
@coderabbitai a nullish coalescing operator has already been added in 82bebf4
There was a problem hiding this comment.
@mlbrgl Thank you for the update. I've verified the changes in commit 82bebf4c1d05b7aee20cead971f37cd50f207633, and the nullish coalescing operator has indeed been added to the isPostCitable function. Your implementation now correctly handles potential null or undefined values for subcategories.
🎉
🐇✨
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the changes made in the specified commit for the `isPostCitable` function
git show 82bebf4c1d05b7aee20cead971f37cd50f207633 -- db/wpdb.ts
Length of output: 5661
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| export interface EntryMeta { | ||
| slug: string | ||
| title: string | ||
| excerpt: string | ||
| kpi: string | ||
| } | ||
|
|
||
| export interface CategoryWithEntries { | ||
| name: string | ||
| slug: string | ||
| entries: EntryMeta[] | ||
| subcategories: CategoryWithEntries[] | ||
| subcategories?: CategoryWithEntries[] | ||
| } | ||
|
|
||
| export enum GraphDocumentType { | ||
| Topic = "topic", | ||
| Article = "article", |
There was a problem hiding this comment.
Tip
Codebase Verification
The search for .excerpt yielded multiple results across various files in the codebase, indicating that the excerpt property is indeed used in several places. However, there were no results for .kpi, suggesting that this property might not be used or is less prevalent.
Given the results, it is clear that the removal of the excerpt property from the EntryMeta interface could potentially impact the functionality in the files where .excerpt is referenced. This needs to be addressed to ensure that the removal does not cause any unintended side effects.
The review comment about verifying the impact of removing excerpt and kpi from EntryMeta is therefore substantiated by the search results for .excerpt. The absence of .kpi in the search results does not necessarily mean it is not used, as it could be referenced in a way that was not captured by the search pattern.
Based on the findings, the appropriate response is to flag the potential issues with the removal of the excerpt property.
Analysis chain
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-11]
Verify that the removal of excerpt and kpi from EntryMeta does not negatively impact other parts of the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of EntryMeta properties that have been removed.
ast-grep --lang typescript --pattern $'EntryMeta { $$$ .excerpt $$$ }'
ast-grep --lang typescript --pattern $'EntryMeta { $$$ .kpi $$$ }'
Length of output: 1041
Script:
#!/bin/bash
# Correct the pattern syntax and search for usages of EntryMeta properties that have been removed.
ast-grep --lang typescript --pattern $'interface EntryMeta { $$$ }' --exec 'rg "\.excerpt" {}'
ast-grep --lang typescript --pattern $'interface EntryMeta { $$$ }' --exec 'rg "\.kpi" {}'
Length of output: 575
Script:
#!/bin/bash
# Find files that contain the EntryMeta interface definition.
entry_meta_files=$(ast-grep --lang typescript --pattern $'interface EntryMeta { $$$ }' | rg -o '[^ ]+$')
# Search for usages of the removed properties within those files.
for file in $entry_meta_files; do
rg "\.excerpt" $file
rg "\.kpi" $file
done
Length of output: 1109
Script:
#!/bin/bash
# Directly search the entire codebase for references to the removed properties.
rg "\.excerpt"
rg "\.kpi"
Length of output: 1611
mlbrgl
force-pushed
the
static-navigation
branch
from
7fc5a2b to
3fd221f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- baker/siteRenderers.tsx (3 hunks)
- db/wpdb.ts (4 hunks)
- packages/@ourworldindata/types/src/domainTypes/ContentGraph.ts (1 hunks)
- site/SiteNavigation.tsx (1 hunks)
- site/SiteNavigationTopics.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- baker/siteRenderers.tsx
- db/wpdb.ts
- packages/@ourworldindata/types/src/domainTypes/ContentGraph.ts
- site/SiteNavigationTopics.tsx
site/SiteNavigation.tsx
Outdated
| { | ||
| "name": "Human Rights and Democracy", | ||
| "slug": "politics", | ||
| "entries": [], | ||
| "subcategories": [ | ||
| { | ||
| "name": "Human Rights", | ||
| "slug": "human-rights", | ||
| "entries": [ | ||
| { | ||
| "slug": "state-capacity", | ||
| "title": "State Capacity" | ||
| }, | ||
| { | ||
| "slug": "lgbt-rights", | ||
| "title": "LGBT+ Rights" | ||
| }, | ||
| { | ||
| "slug": "women-rights", | ||
| "title": "Women’s Rights" | ||
| }, | ||
| { | ||
| "slug": "child-labor", | ||
| "title": "Child Labor" | ||
| }, | ||
| { | ||
| "slug": "human-rights", | ||
| "title": "Human Rights" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "name": "Democracy and Corruption", | ||
| "slug": "democracy", | ||
| "entries": [ | ||
| { | ||
| "slug": "democracy", | ||
| "title": "Democracy" | ||
| }, | ||
| { | ||
| "slug": "corruption", | ||
| "title": "Corruption" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "name": "Violence and War", | ||
| "slug": "violence-rights", | ||
| "entries": [], | ||
| "subcategories": [ | ||
| { | ||
| "name": "War and Peace", | ||
| "slug": "war-peace", | ||
| "entries": [ | ||
| { | ||
| "slug": "biological-and-chemical-weapons", | ||
| "title": "Biological and Chemical Weapons" | ||
| }, | ||
| { | ||
| "slug": "war-and-peace", | ||
| "title": "War and Peace" | ||
| }, | ||
| { | ||
| "slug": "military-personnel-spending", | ||
| "title": "Military Personnel and Spending" | ||
| }, | ||
| { | ||
| "slug": "terrorism", | ||
| "title": "Terrorism" | ||
| }, | ||
| { | ||
| "slug": "nuclear-weapons", | ||
| "title": "Nuclear Weapons" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "name": "Violence", | ||
| "slug": "violence", | ||
| "entries": [ | ||
| { | ||
| "slug": "state-capacity", | ||
| "title": "State Capacity" | ||
| }, | ||
| { | ||
| "slug": "violence-against-rights-for-children", | ||
| "title": "Violence Against Children and Children’s Rights" | ||
| }, | ||
| { | ||
| "slug": "homicides", | ||
| "title": "Homicides" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
The SiteNavigationStatic object has been added to provide a static representation of site navigation. This is a large object literal with nested structures. Ensure that:
- The object structure aligns with the intended navigation schema.
- There are no duplicate slugs or titles within the same category level, which could cause navigation issues.
- The object is properly formatted and maintains a consistent style for readability.
- Consider breaking down this large object into smaller chunks or separate files if it becomes too unwieldy to manage within a single file.
// Example of breaking down the object into smaller parts for maintainability
export const PopulationCategory: CategoryWithEntries = {
name: "Population and Demographic Change",
slug: "population",
entries: [],
subcategories: [
// ...subcategories
],
};
export const HealthCategory: CategoryWithEntries = {
name: "Health",
slug: "health",
entries: [],
subcategories: [
// ...subcategories
],
};
// ... other categories
export const SiteNavigationStatic: { categories: CategoryWithEntries[] } = {
categories: [
PopulationCategory,
HealthCategory,
// ... other categories
],
};
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (3)
- db/wpdb.ts (4 hunks)
- site/SiteNavigation.tsx (1 hunks)
- site/SiteNavigationTopics.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- db/wpdb.ts
- site/SiteNavigationTopics.tsx
| { | ||
| name: "Human Rights and Democracy", | ||
| slug: "politics", | ||
| entries: [], | ||
| subcategories: [ | ||
| { | ||
| name: "Human Rights", | ||
| slug: "human-rights", | ||
| entries: [ | ||
| { | ||
| slug: "state-capacity", | ||
| title: "State Capacity", | ||
| }, | ||
| { | ||
| slug: "lgbt-rights", | ||
| title: "LGBT+ Rights", | ||
| }, | ||
| { | ||
| slug: "women-rights", | ||
| title: "Women’s Rights", | ||
| }, | ||
| { | ||
| slug: "child-labor", | ||
| title: "Child Labor", | ||
| }, | ||
| { | ||
| slug: "human-rights", | ||
| title: "Human Rights", | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| name: "Democracy and Corruption", | ||
| slug: "democracy", | ||
| entries: [ | ||
| { | ||
| slug: "democracy", | ||
| title: "Democracy", | ||
| }, | ||
| { | ||
| slug: "corruption", | ||
| title: "Corruption", | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| name: "Violence and War", | ||
| slug: "violence-rights", | ||
| entries: [], | ||
| subcategories: [ | ||
| { | ||
| name: "War and Peace", | ||
| slug: "war-peace", | ||
| entries: [ | ||
| { | ||
| slug: "biological-and-chemical-weapons", | ||
| title: "Biological and Chemical Weapons", | ||
| }, | ||
| { | ||
| slug: "war-and-peace", | ||
| title: "War and Peace", | ||
| }, | ||
| { | ||
| slug: "military-personnel-spending", | ||
| title: "Military Personnel and Spending", | ||
| }, | ||
| { | ||
| slug: "terrorism", | ||
| title: "Terrorism", | ||
| }, | ||
| { | ||
| slug: "nuclear-weapons", | ||
| title: "Nuclear Weapons", | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| name: "Violence", | ||
| slug: "violence", | ||
| entries: [ | ||
| { | ||
| slug: "state-capacity", | ||
| title: "State Capacity", | ||
| }, | ||
| { | ||
| slug: "violence-against-rights-for-children", | ||
| title: "Violence Against Children and Children’s Rights", | ||
| }, | ||
| { | ||
| slug: "homicides", | ||
| title: "Homicides", | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has identified multiple instances of non-unique slugs within the SiteNavigationStatic object, which aligns with the concern raised in the review comment about ensuring slug uniqueness. This could potentially lead to navigation issues.
Analysis chain
The SiteNavigationStatic object has been significantly expanded. Ensure that all slugs within this object are unique to prevent navigation issues. Consider adding a test or a build-time check to enforce the uniqueness of slugs.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all slugs in SiteNavigationStatic are unique
rg --type js "slug: \"[a-z0-9-]+\"" site/SiteNavigation.tsx | sort | uniq -c | grep -v " 1 "
Length of output: 1622
The static navigation structure is extensive and deeply nested. Ensure that the structure is well-documented and that there is a clear process for developers to update it when necessary. This is important for maintainability, especially since the navigation is expected to be managed by developers.
There was a problem hiding this comment.
can you list the non-unique slugs identified?
There was a problem hiding this comment.
@mlbrgl The non-unique slugs identified in the SiteNavigationStatic object are as follows:
air-pollution
alcohol-consumption
child-labor
child-mortality
clean-water-sanitation
corruption
democracy
economic-inequality
energy
energy-access
environmental-impacts-of-food
financing-education
financing-healthcare
human-rights
indoor-air-pollution
internet
land-use
lead-pollution
life-expectancy
migration
military-personnel-spending
obesity
outdoor-air-pollution
research-and-development
sanitation
state-capacity
technological-change
time-use
tourism
water-access
working-hours
These slugs appear more than once in the navigation structure and should be made unique to prevent navigation issues.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Extract slugs from SiteNavigationStatic, sort them, and identify duplicates
rg --type js "slug: \"[a-z0-9-]+\"" site/SiteNavigation.tsx | \
sed -E 's/.*slug: "([^"]+)".*/\1/' | \
sort | \
uniq -d
Length of output: 598
There was a problem hiding this comment.
it's fine for slugs to appear in multiple places in the navigation, since topics can appear under multiple top-level categories.
There was a problem hiding this comment.
@mlbrgl Understood, the reuse of slugs across different top-level categories is by design to accommodate topics appearing under multiple categories. No changes are required regarding slug uniqueness. Thank you for the clarification.
🐰✨
New Learnings Added
User: mlbrgl
PR: owid/owid-grapher#3126
File: site/SiteNavigation.tsx:251-1094
Timestamp: 2024-01-23T13:39:23.643Z
Learning: Slugs in the `SiteNavigationStatic` object can be non-unique across different top-level categories because topics can appear under multiple categories. This is intentional and acceptable within the navigation structure.
|
@mlbrgl just a quick heads up that I added one tiny commit to remove an unused import so you might want to rebase your later changes |
This replaces the dynamic Wordpress navigation, derived from the categories taxonomy, with a static navigation object.
Until we find a more permanent place and editing UI for the navigation, I opted for a simple JS object, which gives us type checking for free.
I don't expect this data structure to evolve regularly in the near future (it hasn't over the past few years), so it is probably not worth complexifying the setup with a JSON-based alternative and training authors to update it. This will likely remain a developer task.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores