feat(web): Organization page with "standalone" theme - Level 1 sitemap #17068
Conversation
WalkthroughThe pull request introduces several enhancements to the handling of organization pages within the application. A new page type, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
libs/cms/src/lib/dto/getOrganizationPageStandaloneSitemap.input.ts (2)
6-18: Add JSDoc documentation and consider using a union type for lang.While the implementation is functional, adding documentation would improve maintainability. Also, the
langfield could benefit from a more specific type.Consider adding documentation and improving type safety:
+/** + * Input type for fetching organization page standalone sitemap data + */ @InputType() export class GetOrganizationPageStandaloneSitemapLevel1Input { + /** The slug of the organization page */ @Field() @IsString() organizationPageSlug!: string + /** The slug of the category */ @Field() @IsString() categorySlug!: string + /** The locale for content, defaults to Icelandic */ @Field(() => String) @IsString() - lang: ElasticsearchIndexLocale = 'is' + lang: Extract<ElasticsearchIndexLocale, 'is' | 'en'> = 'is' }
6-18: Consider using constructor initialization instead of non-null assertions.The non-null assertion operator (!) can be replaced with constructor-based initialization for better type safety.
Consider this alternative implementation:
@InputType() export class GetOrganizationPageStandaloneSitemapLevel1Input { @Field() @IsString() - organizationPageSlug!: string + organizationPageSlug: string @Field() @IsString() - categorySlug!: string + categorySlug: string @Field(() => String) @IsString() lang: ElasticsearchIndexLocale = 'is' + + constructor(organizationPageSlug: string, categorySlug: string) { + this.organizationPageSlug = organizationPageSlug; + this.categorySlug = categorySlug; + } }libs/cms/src/lib/models/organizationPageStandaloneSitemap.model.ts (2)
4-14: Consider using constructor initialization instead of non-null assertionsWhile the TypeScript implementation is good, we can make it more robust by initializing required fields through a constructor instead of using non-null assertions (!).
Here's a suggested improvement:
@ObjectType() export class OrganizationPageStandaloneSitemapLink { @Field() - label!: string + label: string @Field() - href!: string + href: string @Field(() => String, { nullable: true }) description?: string | null + + constructor(label: string, href: string, description?: string | null) { + this.label = label; + this.href = href; + this.description = description; + } }
16-20: Consider using constructor initialization for consistencySimilar to the previous class, we can improve type safety by using constructor initialization.
Here's a suggested improvement:
@ObjectType() export class OrganizationPageStandaloneSitemap { @CacheField(() => [OrganizationPageStandaloneSitemapLink]) - childLinks!: OrganizationPageStandaloneSitemapLink[] + childLinks: OrganizationPageStandaloneSitemapLink[] + + constructor(childLinks: OrganizationPageStandaloneSitemapLink[]) { + this.childLinks = childLinks; + } }apps/web/screens/Organization/Standalone/ParentSubpage.tsx (1)
71-76: Good SEO implementation, consider adding meta description.The title implementation follows SEO best practices by providing context and hierarchy. However, we could enhance SEO further.
Consider adding a meta description to improve search engine snippets:
<StandaloneLayout organizationPage={organizationPage} seo={{ title: `${subpage.title} | ${organizationPage.title}`, + description: subpage.description || organizationPage.description, }} >apps/web/pages/s/[...slugs]/index.tsx (1)
178-178: Consider extracting theme check to a utility functionWhile the boolean variable improves readability, consider extracting this check into a reusable utility function for consistency across the codebase.
+ const isStandaloneTheme = (page: typeof organizationPage): boolean => + page.theme === 'standalone' - const isStandaloneTheme = organizationPage.theme === 'standalone' + const hasStandaloneTheme = isStandaloneTheme(organizationPage)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/web/pages/s/[...slugs]/index.tsx(8 hunks)apps/web/screens/Organization/Standalone/Level1Sitemap.tsx(1 hunks)apps/web/screens/Organization/Standalone/ParentSubpage.tsx(1 hunks)apps/web/screens/queries/Organization.tsx(1 hunks)libs/cms/src/lib/cms.contentful.service.ts(2 hunks)libs/cms/src/lib/cms.resolver.ts(2 hunks)libs/cms/src/lib/dto/getOrganizationPageStandaloneSitemap.input.ts(1 hunks)libs/cms/src/lib/models/organizationPageStandaloneSitemap.model.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
apps/web/pages/s/[...slugs]/index.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/Organization/Standalone/Level1Sitemap.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/Organization/Standalone/ParentSubpage.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/queries/Organization.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/cms/src/lib/cms.contentful.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/cms/src/lib/cms.resolver.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/cms/src/lib/dto/getOrganizationPageStandaloneSitemap.input.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/cms/src/lib/models/organizationPageStandaloneSitemap.model.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (14)
libs/cms/src/lib/dto/getOrganizationPageStandaloneSitemap.input.ts (1)
1-5: LGTM! Clean imports and proper decorator usage.
The imports are specific and well-organized, following tree-shaking best practices. The @InputType() decorator correctly identifies this as a GraphQL input type.
libs/cms/src/lib/models/organizationPageStandaloneSitemap.model.ts (2)
1-2: LGTM! Well-structured imports
The imports are minimal and specific, which is good for tree-shaking optimization.
16-19: Good use of caching for the array field
The @CacheField decorator is appropriately used for the childLinks array, which can help optimize performance when fetching sitemap data.
apps/web/screens/queries/Organization.tsx (1)
434-446: LGTM! The query follows GraphQL and TypeScript best practices.
The new query is well-structured, follows naming conventions, and aligns with the PR objectives for implementing a Level 1 sitemap.
Let's verify the input type exists and matches the query structure:
✅ Verification successful
Input type exists and is properly integrated across the codebase
The input type GetOrganizationPageStandaloneSitemapLevel1Input is well-defined in libs/cms/src/lib/dto/getOrganizationPageStandaloneSitemap.input.ts with:
- Required fields:
organizationPageSlug,categorySlug - Optional field:
lang(defaults to 'is')
The type is properly imported and used in:
- GraphQL resolver (
cms.resolver.ts) - Contentful service (
cms.contentful.service.ts) - Web queries (
Organization.tsx)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the input type exists and its structure matches the query
# Expected: Find the input type definition with matching fields
# Search for the input type definition
ast-grep --pattern 'class GetOrganizationPageStandaloneSitemapLevel1Input {
$$$
}'
# Search for any references to ensure it's properly imported
rg "GetOrganizationPageStandaloneSitemapLevel1Input"
Length of output: 2045
apps/web/pages/s/[...slugs]/index.tsx (5)
34-36: LGTM: Clean import declaration
The import follows TypeScript best practices by explicitly importing types and maintaining consistent import structure.
52-52: LGTM: Consistent enum value addition
The new enum value follows the established naming convention and pattern.
69-71: LGTM: Consistent pageMap implementation
The new page type mapping follows the established pattern of the pageMap object.
100-103: LGTM: Well-typed Props interface extension
The Props interface extension maintains type safety and follows the established pattern.
251-256: 🛠️ Refactor suggestion
Verify link matching logic robustness
The current link matching logic using split('/').pop() might be fragile if:
- The href format changes
- The href contains trailing slashes
- The href is empty
Consider using a more robust URL parsing approach.
- slugs[1] === link.href.split('/').pop()
+ const linkPath = new URL(link.href, 'http://dummy.com').pathname
+ slugs[1] === linkPath.split('/').filter(Boolean).pop()libs/cms/src/lib/cms.resolver.ts (2)
131-132: LGTM!
The imports follow the established naming conventions and are properly organized with related imports.
723-731: Verify error handling in the service layer.
The resolver implementation follows the established patterns and is properly typed. However, ensure that the service method getOrganizationPageStandaloneSitemapLevel1 implements proper error handling for cases such as:
- Non-existent organization pages
- Invalid input parameters
- Content fetching failures
✅ Verification successful
Let me gather more information about the error handling patterns in the service.
Let me check for the error handler utility since we saw a reference to errorHandler in the code.
Error handling is properly implemented in the service layer
The service method getOrganizationPageStandaloneSitemapLevel1 implements comprehensive error handling through:
- A global error handler that logs errors and throws ApolloError
- Proper null checks for non-existent data (
if (!tree) return null) - Contentful repository calls wrapped with
.catch(errorHandler('getOrganizationPageStandaloneSitemapLevel1')) - Consistent error handling pattern across organization-related methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling implementation in the service layer
# Search for the service method implementation and its error handling
ast-grep --pattern 'getOrganizationPageStandaloneSitemapLevel1($$$) {
$$$
try {
$$$
} catch {
$$$
}
$$$
}'
# Search for similar methods to verify consistent error handling patterns
rg -A 5 "getOrganizationPage.*\{" libs/cms/src/lib/cms.contentful.service.ts
Length of output: 571
Script:
#!/bin/bash
# Search for the complete implementation of getOrganizationPageStandaloneSitemapLevel1
rg -B 5 -A 20 "getOrganizationPageStandaloneSitemapLevel1" libs/cms/src/lib/cms.contentful.service.ts
# Look for error handling patterns in similar methods within the service
rg -A 10 "async get.*Organization.*\{" libs/cms/src/lib/cms.contentful.service.ts
# Check if there's a common error handling utility or wrapper
rg -l "try.*catch" libs/cms/src/lib/cms.contentful.service.ts
Length of output: 2937
Script:
#!/bin/bash
# Search for the errorHandler implementation
rg -B 5 -A 10 "errorHandler.*=.*" libs/cms/src/lib
# Check for any custom error types or interfaces
rg -l "interface.*Error|type.*Error|class.*Error" libs/cms/src/lib
# Look for error-related imports
rg "import.*Error" libs/cms/src/lib/cms.contentful.service.ts
Length of output: 1941
apps/web/screens/Organization/Standalone/Level1Sitemap.tsx (1)
1-154: Overall code implementation looks good
The StandaloneLevel1Sitemap component and its getProps method are well-structured and adhere to Next.js and TypeScript best practices. The code is clean, readable, and makes optimal use of TypeScript for type safety. Server-side data fetching is efficiently handled, and error cases are thoughtfully managed.
libs/cms/src/lib/cms.contentful.service.ts (2)
91-94: Imports added appropriately
The necessary imports for the new functionality have been correctly added.
1183-1270: Method implementation follows TypeScript best practices
The getOrganizationPageStandaloneSitemapLevel1 method is correctly implemented with appropriate TypeScript types and aligns with the coding guidelines regarding reusability and type definitions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17068 +/- ##
==========================================
- Coverage 35.69% 35.68% -0.01%
==========================================
Files 6938 6941 +3
Lines 147122 147218 +96
Branches 41824 41849 +25
==========================================
+ Hits 52516 52538 +22
- Misses 94606 94680 +74
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 11 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (5) |
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
libs/cms/src/lib/cms.contentful.service.ts (4)
1183-1191: Add input validationConsider validating the input parameters before constructing the query to ensure robustness.
async getOrganizationPageStandaloneSitemapLevel1( input: GetOrganizationPageStandaloneSitemapLevel1Input, ): Promise<OrganizationPageStandaloneSitemap | null> { + if (!input?.organizationPageSlug || !input?.categorySlug || !input?.lang) { + return null + } const params = { content_type: 'organizationPage', 'fields.slug': input.organizationPageSlug, select: 'fields.sitemap', limit: 1, }
1203-1211: Optimize category finding with early returnsConsider restructuring the code to avoid unnecessary processing when no category is found.
+ if (!tree.childNodes?.length) { + return null + } const category = tree.childNodes.find( (node) => node.type === SitemapTreeNodeType.CATEGORY && node.slug === input.categorySlug, ) if (!category) { return null }
1213-1275: Improve type safety in node mappingConsider using type guards or discriminated unions for better type safety in the node mapping logic.
+ type NodeWithEntry = { + label: string + href: string + entryId: string + } + type NodeResult = + | { label: string; href: string; description?: string } + | NodeWithEntry const entryNodes = new Map<string, NodeWithEntry[]>() const result = { - childLinks: category.childNodes.map((node) => { + childLinks: category.childNodes.map((node): NodeResult => {
1277-1282: Enhance type narrowing in filter predicateConsider using a type predicate to improve type safety in the filter operation.
+ const isResolvedLink = ( + link: typeof result.childLinks[number] + ): link is { label: string; href: string } => { + return !('entryId' in link) || !entryIdsToRemove.includes(link.entryId) + } result.childLinks = result.childLinks.filter( - (link) => !('entryId' in link && entryIdsToRemove.includes(link.entryId)), + isResolvedLink )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/screens/Organization/Standalone/ParentSubpage.tsx(1 hunks)apps/web/screens/queries/Organization.tsx(1 hunks)libs/cms/src/lib/cms.contentful.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/screens/Organization/Standalone/ParentSubpage.tsx
- apps/web/screens/queries/Organization.tsx
🧰 Additional context used
📓 Path-based instructions (1)
libs/cms/src/lib/cms.contentful.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/cms/src/lib/cms.contentful.service.ts (2)
91-94: LGTM: Imports are well-organized
The new imports follow TypeScript best practices and maintain consistency with the existing codebase.
1193-1201: 🛠️ Refactor suggestion
Strengthen null checks for response handling
Add more defensive checks when accessing nested fields.
const response = await this.contentfulRepository
.getLocalizedEntries<types.IOrganizationPageFields>(input.lang, params)
.catch(errorHandler('getOrganizationPageStandaloneSitemapLevel1'))
- const tree = response.items?.[0]?.fields.sitemap?.fields.tree as SitemapTree
+ const tree = response.items?.[0]?.fields?.sitemap?.fields?.tree as SitemapTree | undefined
if (!tree) {
return null
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/web/screens/Organization/Standalone/Level1Sitemap.tsx (2)
1-9: Consolidate island-ui/core importsThe imports from '@island.is/island-ui/core' are split into two separate statements. Consider consolidating them for better maintainability.
import { CategoryCard, GridColumn, GridContainer, GridRow, Stack, + Text, } from '@island.is/island-ui/core' -import { Text } from '@island.is/island-ui/core'
143-152: Consider using Array.prototype.find with optional chainingThe current implementation could be simplified using optional chaining to make it more concise and readable.
- const categoryTitle = organizationPage?.topLevelNavigation?.links.find( - (link) => categorySlug === link.href.split('/').pop(), - )?.label + const categoryTitle = organizationPage?.topLevelNavigation?.links?.find( + (link) => link.href?.split('/')?.pop() === categorySlug + )?.labelapps/web/components/LanguageToggler/LanguageToggler.tsx (1)
Line range hint
1-240: Consider splitting the component for better maintainability.To improve the component's architecture and maintainability, consider:
- Extracting the GraphQL query logic into a custom hook (e.g.,
useContentSlug)- Moving the
ButtonElementcomponent to a separate file- Splitting the language toggle and content fetching responsibilities
This would make the code more modular and easier to test.
Example structure:
// hooks/useContentSlug.ts export const useContentSlug = () => { const client = useApolloClient() const getContentSlug = async (contentfulId: string) => { return client.query<GetContentSlugQuery, GetContentSlugQueryVariables>({ query: GET_CONTENT_SLUG, variables: { input: { id: contentfulId } }, }) } return { getContentSlug } } // components/LanguageToggler/ButtonElement.tsx export const ButtonElement: FC<ButtonElementProps> = ({ ... }) // components/LanguageToggler/LanguageToggler.tsx // Main component with reduced complexity
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/components/LanguageToggler/LanguageToggler.tsx(1 hunks)apps/web/screens/Organization/Standalone/Level1Sitemap.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/web/components/LanguageToggler/LanguageToggler.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/Organization/Standalone/Level1Sitemap.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (5)
apps/web/screens/Organization/Standalone/Level1Sitemap.tsx (5)
34-46: LGTM! Well-structured type definitions
The types and interfaces are well-defined with proper TypeScript types and nullability handling.
55-57: LGTM! Good SEO implementation
The SEO title is properly constructed by combining the category title and organization page title.
68-78: Use unique keys instead of indices in list rendering
Using array indices as React keys can lead to issues with component reconciliation.
- <GridColumn span={GRID_COLUMN_SPAN} key={index}>
+ <GridColumn span={GRID_COLUMN_SPAN} key={href}>94-95: Validate presence of required slugs before proceeding
The destructuring of slugs lacks validation, which could lead to runtime errors.
const [organizationPageSlug, categorySlug] = (query.slugs ?? []) as string[]
+if (!organizationPageSlug || !categorySlug) {
+ throw new CustomNextError(400, 'Missing required slug parameters')
+}103-130: LGTM! Efficient parallel data fetching
The use of Promise.all for parallel data fetching is a good practice for optimizing performance. The implementation also includes proper error handling and type safety.
kodiakhq
bot
deleted the
feature/web-organization-page-standalone-level1-sitemap
branch
Organization page with "standalone" theme - Level 1 sitemap
What
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
Release Notes
New Features
STANDALONE_LEVEL1_SITEMAP, enhancing sitemap rendering capabilities.StandaloneLevel1Sitemapcomponent for displaying organization-level sitemaps.StandaloneParentSubpagewith dynamic title generation.LanguageTogglercomponent to provide clearer messaging regarding translation availability.Bug Fixes
These updates enhance navigation, improve SEO, and expand the application's functionality related to organization pages.