feat(web): Organization parent subpage #17022
Conversation
WalkthroughThe pull request introduces the Changes
Possibly related PRs
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
|
Datadog ReportAll test runs ✅ 11 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
libs/cms/src/lib/dto/getOrganizationParentSubpage.input.ts (3)
7-9: Add field description and validation for the slug.Consider enhancing the field with:
- A description for better GraphQL documentation
- Additional validation rules for the slug format
- @Field() + @Field(() => String, { + description: 'The unique slug identifier for the parent subpage' + }) @IsString() + @Matches(/^[a-z0-9-]+$/, { + message: 'Slug must contain only lowercase letters, numbers, and hyphens' + }) slug!: string
11-13: Add field description and validation for the organization page slug.Consider enhancing the field with:
- A description for better GraphQL documentation
- Additional validation rules for the slug format
- @Field() + @Field(() => String, { + description: 'The unique slug identifier for the organization page' + }) @IsString() + @Matches(/^[a-z0-9-]+$/, { + message: 'Organization page slug must contain only lowercase letters, numbers, and hyphens' + }) organizationPageSlug!: string
15-17: Add field description and enum validation for the language.Consider enhancing the field with:
- A description for better GraphQL documentation
- Enum validation to ensure valid locale values
- @Field(() => String) + @Field(() => String, { + description: 'The language locale for the content. Defaults to Icelandic ("is")' + }) @IsString() + @IsEnum(ElasticsearchIndexLocale, { + message: 'Language must be a valid ElasticsearchIndexLocale value' + }) lang: ElasticsearchIndexLocale = 'is'libs/cms/src/lib/models/organizationParentSubpage.model.ts (2)
1-5: Consider exporting interface types for better reusabilityTo enhance reusability across different NextJS apps, consider exporting the interface types that define the shape of the data expected by the mapping function.
Add these exports at the top of the file:
export interface IOrganizationSubpageLink { label: string; href: string; } export interface IOrganizationParentSubpage { sys: { id: string; locale: string }; fields: { title: string; slug: string; pages: Array<{ fields: { title: string; slug: string; organizationPage: { fields: { slug: string; }; }; }; }>; image?: any; }; }
38-44: Simplify URL construction logicThe URL construction in the childLinks mapping is complex and could be made more maintainable.
Consider extracting the URL construction logic:
+ const buildPageUrl = (locale: string, orgSlug: string, parentSlug: string, pageSlug: string) => + `/${getOrganizationPageUrlPrefix(locale)}/${orgSlug}/${parentSlug}/${pageSlug}`; childLinks: fields.pages?.map((page) => ({ label: page.fields.title, - href: `/${getOrganizationPageUrlPrefix(sys.locale)}/${ - page.fields.organizationPage.fields.slug - }/${fields.slug}/${page.fields.slug}`, + href: buildPageUrl( + sys.locale, + page.fields.organizationPage.fields.slug, + fields.slug, + page.fields.slug + ), })) ?? [],apps/web/screens/GenericList/OrganizationSubPageGenericListItem.tsx (1)
23-25: Add JSDoc comment to document the type's purposeThe type definition is well-structured, but adding documentation would improve maintainability.
+/** + * Screen context extended with organization page data from GraphQL. + * Used to provide organization-specific context to the generic list item component. + */ type OrganizationSubPageGenericListItemScreenContext = ScreenContext & { organizationPage?: Query['getOrganizationPage'] }apps/web/screens/queries/Organization.tsx (1)
420-432: Consider enhancing error handling and reusability.To align with best practices and improve maintainability:
- Add error handling fields to handle potential query failures gracefully
- Consider extracting common fields into fragments for reusability
Consider updating the query to:
export const GET_ORGANIZATION_PARENT_SUBPAGE_QUERY = gql` query GetOrganizationParentSubpageQuery( $input: GetOrganizationParentSubpageInput! ) { getOrganizationParentSubpage(input: $input) { + success + error { + message + code + } title childLinks { label href } } } `apps/web/screens/Organization/OrganizationEvents/OrganizationEventArticle.tsx (1)
Line range hint
341-345: Improve error message accuracyThe error message uses
query.slugwhich doesn't exist in the destructured parameters. It should useorganizationPageSlugfor consistency with the code.throw new CustomNextError( 404, - `Could not find organization page with slug: ${query.slug}`, + `Could not find organization page with slug: ${organizationPageSlug}`, )apps/web/screens/Organization/SubPage.tsx (1)
64-71: Consider adding aria-level for better accessibilityWhile the dynamic title variant is well implemented, consider adding
aria-levelto ensure proper heading hierarchy when using h2 variant, especially for screen readers.<Text variant={subpageTitleVariant} as={subpageTitleVariant} + aria-level={subpageTitleVariant === 'h1' ? '1' : '2'} >Also applies to: 138-141
apps/web/pages/s/[...slugs]/index.tsx (3)
167-168: Consider centralizing the 'STANDALONE_THEME' constantTo enhance maintainability, consider moving
STANDALONE_THEMEto a shared constants file or configuration module.
241-249: Refactor repeated theme check to reduce duplicationThe check for
organizationPage.theme === STANDALONE_THEMEis repeated. Consider refactoring this logic into a helper function to avoid duplication.
295-303: Duplicate handling of standalone themeSimilar logic for handling the standalone theme is present here. Refactoring could improve readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
apps/web/pages/s/[...slugs]/index.tsx(9 hunks)apps/web/screens/GenericList/OrganizationSubPageGenericListItem.tsx(2 hunks)apps/web/screens/Organization/OrganizationEvents/OrganizationEventArticle.tsx(3 hunks)apps/web/screens/Organization/OrganizationNews/OrganizationNewsArticle.tsx(3 hunks)apps/web/screens/Organization/Standalone/ParentSubpage.tsx(1 hunks)apps/web/screens/Organization/SubPage.tsx(6 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/getOrganizationParentSubpage.input.ts(1 hunks)libs/cms/src/lib/models/organizationParentSubpage.model.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
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/GenericList/OrganizationSubPageGenericListItem.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/OrganizationEvents/OrganizationEventArticle.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/OrganizationNews/OrganizationNewsArticle.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/Organization/SubPage.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/getOrganizationParentSubpage.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/organizationParentSubpage.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 (28)
libs/cms/src/lib/dto/getOrganizationParentSubpage.input.ts (1)
5-6: LGTM! Well-structured input type class.
The class is properly exported and follows TypeScript naming conventions, making it reusable across different NextJS apps.
libs/cms/src/lib/models/organizationParentSubpage.model.ts (3)
7-14: LGTM! Well-structured GraphQL object type
The class definition is clean, with proper use of decorators and TypeScript features. The required fields are correctly marked as non-nullable.
31-47: Verify locale handling across the application
The function uses sys.locale for URL construction. Ensure that this locale handling is consistent with the rest of the application.
✅ Verification successful
Based on the search results, I can now generate the final response as the verification is complete:
Locale handling is consistent across the application
The usage of sys.locale in URL construction through getOrganizationPageUrlPrefix is consistent throughout the codebase:
- All organization-related URLs use the same pattern via
getOrganizationPageUrlPrefix - The locale handling utility correctly manages the 'is-IS' to 'is' conversion across different models
- The implementation aligns with other locale-aware components in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for locale usage patterns in URL construction
rg -A 3 "getOrganizationPageUrlPrefix.*locale" --type ts
# Search for other instances of sys.locale usage
ast-grep --pattern 'sys.locale'
Length of output: 3643
16-29: Verify cache invalidation strategy for childLinks
The @CacheField decorator is used for the childLinks array. Ensure that cache invalidation is properly handled when child pages are updated or reordered.
apps/web/screens/GenericList/OrganizationSubPageGenericListItem.tsx (2)
4-4: LGTM: Proper type imports enhance type safety
The addition of GraphQL Query type and explicit type imports from layout and screen contexts follows TypeScript best practices.
Also applies to: 7-8
28-29: Verify getProps implementation for context handling
While the component's type signature is correctly updated to use the new context type, we should verify if getProps needs to be updated to properly populate the organizationPage in the context.
Consider implementing error boundaries to handle cases where organizationPage data fails to load.
✅ Verification successful
No update needed for getProps implementation
The getProps implementation is correct as is. The organizationPage context is already being properly handled:
- The parent component (Screen) fetches and adds
organizationPageto the context viamodifiedContext = { ...context, organizationPage } - This context is passed to
OrganizationSubPageGenericListItem.getProps - The component correctly receives this context through the
SubPage.getPropscall, which is part of theparentProps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other components to verify consistency
ast-grep --pattern 'Screen<$_, OrganizationSubPageGenericListItemScreenContext>'
Length of output: 82
Script:
#!/bin/bash
# Search for getProps implementation in the component
ast-grep --pattern 'getProps($$$) {
$$$
}'
# Search for OrganizationSubPageGenericListItemScreenContext usage
rg "OrganizationSubPageGenericListItemScreenContext" -A 5
# Search for similar organization page context patterns
rg "organizationPage.*Context" -A 5
Length of output: 1429
Script:
#!/bin/bash
# Let's check the entire file content to understand the component structure and getProps implementation
rg -l "OrganizationSubPageGenericListItem" | xargs cat
# Search for any static getProps implementations in the codebase
ast-grep --pattern 'static getProps($$$) {
$$$
}'
Length of output: 12435
apps/web/screens/queries/Organization.tsx (2)
420-432: LGTM! Query structure follows conventions.
The new GraphQL query is well-structured and follows the existing naming and input parameter conventions.
420-432: Verify query implementation and usage.
Let's ensure all required components are in place for this query to function correctly.
✅ Verification successful
Query implementation is complete and properly integrated
The verification shows that all required components are in place:
- Input type is properly defined with required fields in
getOrganizationParentSubpage.input.ts - Resolver is implemented in
cms.resolver.tsand correctly uses the service - Service implementation in
cms.contentful.service.tshandles the content fetching - Query is correctly used in
ParentSubpage.tsxcomponent with proper variables
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of the new GraphQL query
# Check for input type definition
echo "Checking for input type definition..."
ast-grep --pattern 'input GetOrganizationParentSubpageInput {
$$$
}'
# Check for query usage in components
echo "Checking for query usage in components..."
rg -l "GET_ORGANIZATION_PARENT_SUBPAGE_QUERY" --type tsx
# Check for resolver implementation
echo "Checking for resolver implementation..."
rg -l "getOrganizationParentSubpage" --type ts
Length of output: 648
Script:
#!/bin/bash
# Let's check the implementation details and input type definition with proper file extensions
# Check input type definition in GraphQL schema files
echo "Checking GraphQL schema files..."
fd "\.(graphql|gql)$" -x cat {}
# Check resolver implementation
echo "Checking resolver implementation..."
rg "getOrganizationParentSubpage" -A 5 libs/cms/src/lib/cms.resolver.ts
# Check query usage in React components
echo "Checking query usage..."
rg "GET_ORGANIZATION_PARENT_SUBPAGE_QUERY" -A 5 apps/web/screens/Organization/Standalone/ParentSubpage.tsx
Length of output: 101698
Script:
#!/bin/bash
# Let's check the implementation details in the resolver and component usage
# Check resolver implementation
echo "Checking resolver implementation in cms.resolver.ts..."
rg -A 10 "getOrganizationParentSubpage" libs/cms/src/lib/cms.resolver.ts
# Check component usage
echo "Checking component usage in ParentSubpage.tsx..."
rg -A 10 "GET_ORGANIZATION_PARENT_SUBPAGE_QUERY" apps/web/screens/Organization/Standalone/ParentSubpage.tsx
# Check input type definition in TypeScript files
echo "Checking input type definition..."
rg "GetOrganizationParentSubpageInput" --type ts
Length of output: 2715
Script:
#!/bin/bash
# Let's check the input type definition and service implementation
# Check input type definition
echo "Checking input type definition..."
cat libs/cms/src/lib/dto/getOrganizationParentSubpage.input.ts
# Check service implementation
echo "Checking service implementation..."
rg -A 10 "getOrganizationParentSubpage" libs/cms/src/lib/cms.contentful.service.ts
Length of output: 1574
apps/web/screens/Organization/OrganizationNews/OrganizationNewsArticle.tsx (3)
31-31: LGTM! Type definitions follow best practices
The type-only import and the new context type definition follow TypeScript best practices. The optional organizationPage in the context allows for flexible data passing between pages.
Also applies to: 42-44
46-49: LGTM! Component type signature ensures type safety
The updated Screen generic type parameters properly specify both props and context types, ensuring type safety throughout the component lifecycle.
168-173: Verify the impact of conditional organization page fetching
The conditional fetching of organizationPage could affect other components that expect this data to be fetched here.
Let's verify the usage of this component:
Also applies to: 176-190
apps/web/screens/Organization/OrganizationEvents/OrganizationEventArticle.tsx (3)
144-146: LGTM: Type definition follows best practices
The new type definition properly extends the base ScreenContext and correctly types the optional organization page data, enhancing type safety across the application.
148-151: LGTM: Component signature properly typed
The component signature correctly implements the Screen generic type with both props and context types, following NextJS best practices for type-safe components.
290-309: LGTM: Efficient data fetching optimization
The conditional query execution prevents redundant database calls when organization page data is already available in the context, improving performance.
apps/web/screens/Organization/SubPage.tsx (3)
46-46: LGTM: Type definitions follow NextJS best practices
The addition of ScreenContext and the new SubPageScreenContext type enhance type safety and properly extend the base Screen type.
Also applies to: 217-219
221-221: LGTM: Proper typing of Screen component
The Screen component is correctly typed with props and context, following NextJS best practices for type safety.
Line range hint 370-400: Consider implementing stale-while-revalidate pattern
The conditional query optimization is good. Consider further improving performance by implementing SWR pattern using revalidate in getStaticProps or cache-control headers in getServerSideProps.
libs/cms/src/lib/cms.resolver.ts (2)
129-130: LGTM! Import declarations are well-structured.
The new imports follow the established pattern and are correctly placed with other model/DTO imports.
713-719: LGTM! Verify the implementation in the service layer.
The new resolver method follows the established patterns:
- Uses consistent cache control
- Properly handles nullable return type
- Follows the same parameter passing pattern
Let's verify the service implementation:
✅ Verification successful
Service implementation verified and follows established patterns
The service implementation in CmsContentfulService is well-structured and consistent with the codebase patterns:
- Properly handles input validation through a well-defined
GetOrganizationParentSubpageInputDTO - Uses the Contentful repository with appropriate query parameters
- Implements proper error handling through the
errorHandler - Uses a clean mapping function
mapOrganizationParentSubpageto transform the response - Returns nullable response as specified in the resolver
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the service implementation of getOrganizationParentSubpage
# Check the service implementation
ast-grep --pattern 'getOrganizationParentSubpage(input: $_) {
$$$
}'
# Check for corresponding tests
rg -l "getOrganizationParentSubpage.*test" -g "*.spec.ts"
Length of output: 132
Script:
#!/bin/bash
# Let's try a different approach to find the service implementation
# Search for the service class and method
rg -A 10 "class.*CmsContentfulService"
# Search for the method implementation with more context
rg -A 10 "getOrganizationParentSubpage"
# Look for the input type definition
rg "GetOrganizationParentSubpageInput"
# Check for test files in the cms directory
fd "cms.*spec.ts$"
Length of output: 11807
Script:
#!/bin/bash
# Let's check the input type definition and mapping function
# Check the input type definition
cat libs/cms/src/lib/dto/getOrganizationParentSubpage.input.ts
# Check the mapping function for the response
rg -A 10 "mapOrganizationParentSubpage"
# Check the model definition
rg -A 10 "class OrganizationParentSubpage"
Length of output: 3484
libs/cms/src/lib/cms.contentful.service.ts (1)
89-90: LGTM!
The imports follow the established pattern and are correctly placed with other similar imports.
apps/web/pages/s/[...slugs]/index.tsx (8)
34-36: LGTM!
The new component StandaloneParentSubpage and its props are correctly imported.
48-48: New PageType enum value added
The STANDALONE_PARENT_SUBPAGE enum value is correctly defined and follows the existing pattern.
62-64: Properly map STANDALONE_PARENT_SUBPAGE to its component
The pageMap correctly maps PageType.STANDALONE_PARENT_SUBPAGE to the StandaloneParentSubpage component.
89-92: Updated Props interface with new page type
The Props interface now includes the STANDALONE_PARENT_SUBPAGE with the appropriate props type.
149-159: Correctly fetch 'organizationPage' data
The GraphQL query is well-structured, and the result is properly destructured to obtain organizationPage.
161-164: Appropriate error handling for missing 'organizationPage'
The code correctly throws a CustomNextError with a 404 status when organizationPage is not found.
165-165: Context extended with 'organizationPage'
The modifiedContext includes organizationPage, which ensures that downstream components have access to this data.
169-175: Added routing for standalone frontpage
The routing logic correctly handles the case where slugs.length === 1 and the theme is standalone, returning the STANDALONE_FRONTPAGE.
apps/web/screens/Organization/OrganizationNews/OrganizationNewsArticle.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
apps/web/screens/Organization/Standalone/ParentSubpage.tsx (4)
76-85: Consider enhancing breadcrumb navigationThe breadcrumbs only show the organization title. Consider adding intermediate navigation links to improve user experience and follow navigation best practices.
<Breadcrumbs items={[ { title: organizationPage.title, href: linkResolver('organizationpage', [organizationPage.slug]).href, }, + { + title: parentSubpage.title, + href: linkResolver('organizationpage', [organizationPage.slug, parentSubpage.slug]).href, + }, ]} />
101-106: Move hardcoded strings to translation namespaceThe table of contents title fallback uses hardcoded strings. Consider moving these to the translation namespace for better maintainability and consistency.
tableOfContentsTitle={ - namespace?.['standaloneTableOfContentsTitle'] ?? - activeLocale === 'is' - ? 'Efnisyfirlit' - : 'Table of contents' + namespace?.['standaloneTableOfContentsTitle'] ?? + namespace?.['defaultTableOfContentsTitle'] }
185-191: Enhance error messages with more contextThe error messages could be more specific to help with debugging. Consider including relevant identifiers in the error messages.
if (!getOrganizationPage) { - throw new CustomNextError(404, 'Organization page not found') + throw new CustomNextError( + 404, + `Organization page not found: ${organizationPageSlug}` + ) } if (!getOrganizationParentSubpage) { - throw new CustomNextError(404, 'Organization parent subpage was not found') + throw new CustomNextError( + 404, + `Organization parent subpage not found: ${parentSubpageSlug} for organization ${organizationPageSlug}` + ) }
209-223: Consider implementing caching strategyThe GraphQL query for subpage data could benefit from caching to improve performance, especially for frequently accessed pages.
Consider implementing Apollo Client's caching policies:
query: GET_ORGANIZATION_SUBPAGE_QUERY, fetchPolicy: 'cache-first', nextFetchPolicy: 'cache-only',apps/web/pages/s/[...slugs]/index.tsx (3)
62-64: Consider improving type safety of pageMapWhile the implementation is correct, consider removing the
FC<any>typing and using proper component types for better type safety.-const pageMap: Record<PageType, FC<any>> = { +const pageMap: Record<PageType, { + [PageType.FRONTPAGE]: FC<HomeProps>; + [PageType.STANDALONE_FRONTPAGE]: FC<StandaloneHomeProps>; + [PageType.STANDALONE_PARENT_SUBPAGE]: FC<StandaloneParentSubpageProps>; + // ... other mappings +}> = {
Line range hint
169-302: Consider refactoring routing logic for better maintainabilityThe current implementation has several areas that could be improved:
- Duplicated standalone theme checks
- Deep nesting of conditional statements
- Similar patterns in handling 2 and 3 slugs
Consider refactoring to:
- Extract route handlers into separate functions
- Use early returns to reduce nesting
- Create a route configuration map
Example approach:
const routeHandlers = { standalone: { single: async (context) => ({ type: PageType.STANDALONE_FRONTPAGE, props: await StandaloneHome.getProps(context), }), double: async (context) => ({ type: PageType.STANDALONE_PARENT_SUBPAGE, props: await StandaloneParentSubpage.getProps(context), }), triple: async (context) => ({ type: PageType.STANDALONE_PARENT_SUBPAGE, props: await StandaloneParentSubpage.getProps(context), }), }, default: { // ... similar structure for default theme } }; // In getProps: const handler = routeHandlers[ organizationPage.theme === STANDALONE_THEME ? 'standalone' : 'default' ][slugs.length === 1 ? 'single' : slugs.length === 2 ? 'double' : 'triple']; return { page: await handler(modifiedContext) };
161-163: Enhance error handling with more descriptive messagesConsider improving error handling by:
- Adding more descriptive error messages
- Including logging for debugging
- Differentiating between various error cases
if (!organizationPage) { - throw new CustomNextError(404, 'Organization page was not found') + const error = new CustomNextError( + 404, + `Organization page not found for slug: ${slugs[0]}` + ) + console.error('[OrganizationPage]', error) + throw error }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/pages/s/[...slugs]/index.tsx(9 hunks)apps/web/screens/Organization/Standalone/ParentSubpage.tsx(1 hunks)libs/cms/src/lib/models/organizationParentSubpage.model.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/cms/src/lib/models/organizationParentSubpage.model.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/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."
🔇 Additional comments (6)
apps/web/screens/Organization/Standalone/ParentSubpage.tsx (2)
37-53: Well-structured type definitions with proper TypeScript usage!
The type definitions are comprehensive and maintain type safety through proper typing of all fields. The interface structure clearly defines the component's contract.
1-251: Overall implementation follows NextJS best practices!
The component is well-structured with:
- Proper TypeScript usage throughout
- Effective error handling
- Parallel data fetching
- Responsive layout implementation
- Internationalization support
While there are some suggested improvements, the core implementation is solid and follows best practices.
apps/web/pages/s/[...slugs]/index.tsx (4)
34-36: LGTM: Clean import of new component and types
The import follows NextJS conventions and properly imports both the component and its type definition.
48-48: LGTM: Consistent enum value addition
The new enum value follows the existing naming convention and is appropriately placed within the PageType enum.
89-92: LGTM: Well-typed Props interface extension
The Props interface is correctly extended with the new page type, maintaining consistency with existing patterns.
165-167: LGTM: Improved context handling
Good practice to modify context with organizationPage and define STANDALONE_THEME as a constant.
…sArticle.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
RunarVestmann
added
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/cms/src/lib/cms.contentful.service.ts (1)
1156-1177: Consider adding explicit return type and improving type safety.The implementation follows the established pattern and correctly handles the content fetching. However, there are a few improvements that could be made:
- Add explicit return type annotation
- Use non-null assertion for the response type casting
- async getOrganizationParentSubpage(input: GetOrganizationParentSubpageInput) { + async getOrganizationParentSubpage( + input: GetOrganizationParentSubpageInput + ): Promise<OrganizationParentSubpage | null> { const params = { content_type: 'organizationParentSubpage', 'fields.slug': input.slug, 'fields.organizationPage.sys.contentType.sys.id': 'organizationPage', 'fields.organizationPage.fields.slug': input.organizationPageSlug, limit: 1, } const response = await this.contentfulRepository .getLocalizedEntries<types.IOrganizationParentSubpageFields>( input.lang, params, ) .catch(errorHandler('getOrganizationParentSubpage')) return ( - (response.items as types.IOrganizationParentSubpage[]).map( + (response.items as types.IOrganizationParentSubpage[])!.map( mapOrganizationParentSubpage, )[0] ?? null ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/screens/Organization/OrganizationNews/OrganizationNewsArticle.tsx(3 hunks)libs/cms/src/lib/cms.contentful.service.ts(2 hunks)libs/cms/src/lib/models/organizationParentSubpage.model.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/screens/Organization/OrganizationNews/OrganizationNewsArticle.tsx
- libs/cms/src/lib/models/organizationParentSubpage.model.ts
🧰 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 (1)
libs/cms/src/lib/cms.contentful.service.ts (1)
89-90: LGTM!
The new imports follow the established pattern and are correctly organized with other similar imports.
kodiakhq
bot
deleted the
feature/web-organization-parent-subpage-screen
branch
* First draft * Stop fetching organization page twice * Handle if there is only a single child link * Update apps/web/screens/Organization/OrganizationNews/OrganizationNewsArticle.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Add errorHandler to backend endpoint * Remove question mark * Filter out invalid child links --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Organization parent subpage
What
Why
Screenshots / Gifs
Screen.Recording.2024-11-26.at.13.38.06.mov
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation