feat(web): Organization page - top level navigation links #16793
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the integration of new types and structures related to a sitemap and organization pages. The Changes
Possibly related PRs
Suggested labels
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: 2
🧹 Outside diff range and nitpick comments (8)
libs/shared/types/src/lib/api-cms-domain.ts (3)
20-24: Add JSDoc documentation for the enum.Since this is a shared type that will be used across different applications, please add JSDoc documentation explaining the purpose and usage of each enum value.
+/** + * Represents the different types of nodes that can exist in a sitemap tree. + * @enum {string} + */ export enum SitemapTreeNodeType { + /** Represents a content entry node linking to specific content */ ENTRY = 'entry', + /** Represents a category/section node for grouping related content */ CATEGORY = 'category', + /** Represents a direct URL link node */ URL = 'url', }
26-44: Enhance type safety and documentation.The type structure is well-designed, but could benefit from the following improvements:
- Add JSDoc documentation for the type and its variants
- Consider using a URL type for the
urlproperty- Improve the
primaryLocationdocumentation+/** + * Represents a node in the sitemap tree structure. + * Each node can be one of three types: an entry reference, a category, or a URL. + */ export type SitemapTreeNode = SitemapTree & ( | { type: SitemapTreeNodeType.ENTRY entryId: string - primaryLocation: boolean // Whether the parent nodes above are the "main breadcrumb path" (always true unless the entry is in multiple places in the sitemap) + /** + * Indicates if this is the primary location of the entry in the sitemap. + * Used for generating the main breadcrumb path when content appears in multiple locations. + */ + primaryLocation: boolean } | { type: SitemapTreeNodeType.CATEGORY label: string slug: string description: string } | { type: SitemapTreeNodeType.URL label: string - url: string + url: URL | string } )
46-49: Add documentation and consider id type.The tree structure is well-designed but needs documentation for better maintainability.
+/** + * Represents a tree structure for organizing sitemap nodes. + * Forms the base structure that all sitemap nodes extend. + */ export type SitemapTree = { + /** Unique identifier for the tree node */ id: number + /** Array of child nodes in the tree */ childNodes: SitemapTreeNode[] }Consider using a more specific type for
idif it follows a particular pattern (e.g., auto-increment).apps/contentful-apps/components/sitemap/utils.ts (3)
3-9: LGTM! Consider adding JSDoc comments.The migration to shared types from
@island.is/shared/typesimproves type consistency across the codebase. The use of type aliases maintains backward compatibility.Consider adding JSDoc comments to document the re-exported types for better developer experience:
+/** Tree structure representing a sitemap */ +/** Node within the sitemap tree */ +/** Type of node in the sitemap tree */ export type { Tree, TreeNode, TreeNodeType }
Line range hint
134-238: Consider refactoring theaddNodefunction for better maintainability.The function is handling multiple responsibilities and could benefit from being split into smaller, more focused functions.
Consider these improvements:
- Extract node type-specific logic into separate functions:
async function handleEntryNode(sdk: FieldExtensionSDK, createNew: boolean): Promise<EntryNodeData | null>; async function handleCategoryNode(sdk: FieldExtensionSDK): Promise<CategoryNodeData | null>; async function handleUrlNode(sdk: FieldExtensionSDK): Promise<UrlNodeData | null>;
- Use enum values for type checking:
-if (type === TreeNodeType.ENTRY) +if (type === TreeNodeType.ENTRY as const)
- Add error handling for SDK operations:
try { const entry = await sdk.navigator.openNewEntry(ENTRY_CONTENT_TYPE_ID, { slideIn: { waitForClose: true }, }); // ... rest of the code } catch (error) { console.error('Failed to create new entry:', error); return null; }
Line range hint
63-127: Consider performance optimizations for tree traversal functions.The tree traversal functions are well-implemented but could benefit from performance optimizations for large trees.
Consider these improvements:
- Memoize frequently accessed nodes:
const memoizedFindNode = memoize(findNode, { maxSize: 100, resolver: (root, condition) => JSON.stringify(root.id), });
- Optimize findNodes to use a more efficient traversal strategy:
export const findNodes = ( root: Tree, condition: (otherNode: TreeNode) => boolean, ) => { const nodes: TreeNode[] = []; const queue = [...root.childNodes]; while (queue.length) { const node = queue.shift(); if (condition(node)) { nodes.push(node); } queue.push(...node.childNodes); } return nodes; }apps/web/screens/queries/Organization.tsx (1)
130-135: Update documentation to reflect new navigation structure.Since the PR checklist indicates documentation updates are pending, please ensure the following are documented:
- Purpose and usage of topLevelNavigation
- Structure of navigation links
- Integration with the sitemap field
Would you like me to help draft the documentation updates?
libs/cms/src/lib/models/organizationPage.model.ts (1)
109-111: Ensure correct URL construction without duplicate slashesWhen constructing the
hreffor navigation links, ensure that combininggetOrganizationPageUrlPrefix(sys.locale),slug, andnode.slugdoes not result in duplicate slashes, which could lead to incorrect URLs.Consider trimming any leading or trailing slashes from
slugandnode.slug, or use a URL helper function to safely concatenate paths.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
libs/cms/src/lib/generated/contentfulTypes.d.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
apps/contentful-apps/components/sitemap/utils.ts(1 hunks)apps/web/screens/queries/Organization.tsx(1 hunks)libs/cms/src/lib/models/organizationPage.model.ts(3 hunks)libs/shared/types/src/lib/api-cms-domain.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/contentful-apps/components/sitemap/utils.ts (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/models/organizationPage.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."
libs/shared/types/src/lib/api-cms-domain.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
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 (3)
libs/shared/types/src/lib/api-cms-domain.ts (1)
20-49: Verify usage patterns across the codebase.
The types look well-structured for the organization page navigation. Let's verify their usage patterns.
✅ Verification successful
Let me gather more information about the usage patterns to make a comprehensive assessment.
Types are correctly implemented but have limited usage across the codebase
The types are well-structured but currently have minimal implementation:
- Only imported in
organizationPage.model.tsandsitemap/utils.ts - Single type-specific handling found in
organizationPage.model.tsforCATEGORYtype - No complex breadcrumb implementations using
primaryLocationfound yet - No comprehensive tree traversal functions detected
This suggests these types are part of a new or evolving feature, and their limited usage is expected at this stage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these types are being used across the codebase
echo "Checking imports of SitemapTreeNodeType..."
rg "import.*SitemapTreeNodeType" -A 2
echo "Checking usage of discriminated union patterns..."
ast-grep --pattern 'switch ($node.type) {
$$$
}'
echo "Checking breadcrumb implementations using primaryLocation..."
rg "primaryLocation.*breadcrumb" -B 2 -A 2
Length of output: 1235
Script:
#!/bin/bash
# Check for actual implementations using these types
echo "Checking implementations using SitemapTreeNode..."
rg "SitemapTreeNode" -B 2 -A 2
echo "Checking for any type-specific handling..."
rg "type === SitemapTreeNodeType" -B 2 -A 2
echo "Checking for any functions that process the sitemap tree..."
ast-grep --pattern 'function $_($$_: SitemapTree$_) {
$$$
}'
Length of output: 4041
apps/contentful-apps/components/sitemap/utils.ts (1)
3-7: Verify the impact of type changes.
Let's ensure that the type changes don't break existing usages.
✅ Verification successful
Type changes are properly aliased and used consistently
The verification shows that:
- The types
SitemapTree,SitemapTreeNode, andSitemapTreeNodeTypeare defined inlibs/shared/types/src/lib/api-cms-domain.ts - In
utils.ts, these types are correctly imported and aliased asTree,TreeNode, andTreeNodeType - The aliases are used consistently throughout the file
- Other files in the codebase correctly import and use the original type names
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old type names
echo "Checking for old type usage..."
rg --type typescript "enum TreeNodeType" apps/
rg --type typescript "interface TreeNode" apps/
rg --type typescript "interface Tree" apps/
# Check for proper imports of the new types
echo "Verifying new type imports..."
rg --type typescript "SitemapTree|SitemapTreeNode|SitemapTreeNodeType" apps/
Length of output: 503
Script:
#!/bin/bash
# Search for any remaining usage of the old type names without typescript filter
echo "Checking for old type usage..."
rg "enum TreeNodeType"
rg "interface TreeNode"
rg "interface Tree"
# Check for proper imports of the new types
echo "Verifying new type imports..."
rg "SitemapTree|SitemapTreeNode|SitemapTreeNodeType"
# Check the specific file content
echo "Checking the specific file content..."
cat apps/contentful-apps/components/sitemap/utils.ts
Length of output: 8230
apps/web/screens/queries/Organization.tsx (1)
130-135: LGTM! Verify type definitions.
The new topLevelNavigation field structure is clean and follows GraphQL best practices. The implementation aligns well with the PR objectives for enhancing organization page navigation.
Let's verify the corresponding TypeScript types are properly defined:
✅ Verification successful
Types are properly defined and implemented
The topLevelNavigation field is well-typed in the codebase:
- Defined as
OrganizationPageTopLevelNavigationclass inlibs/cms/src/lib/models/organizationPage.model.ts - Contains a
linksarray ofOrganizationPageTopLevelNavigationLinkobjects withlabelandhreffields - Properly integrated into the
OrganizationPagetype with nullable handling - Implementation matches the GraphQL query structure in
Organization.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for topLevelNavigation type definitions
rg -t typescript "interface.*OrganizationPage|type.*OrganizationPage"
Length of output: 106
Script:
#!/bin/bash
# Search for type definitions related to organization page and navigation
rg "type.*Organization|interface.*Organization|type.*Navigation|interface.*Navigation" --type ts
Length of output: 11622
Script:
#!/bin/bash
# Let's check the specific type definitions in the generated types and GraphQL schema
rg -A 10 "type.*OrganizationPage|interface.*OrganizationPage" libs/cms/src/lib/generated/contentfulTypes.d.ts
Length of output: 586
Script:
#!/bin/bash
# Let's check for GraphQL type definitions related to topLevelNavigation
rg -A 5 "topLevelNavigation" --type ts
Length of output: 2008
Script:
#!/bin/bash
# Let's check for OrganizationPageTopLevelNavigation type definition
rg "OrganizationPageTopLevelNavigation" --type ts -B 2 -A 5
Length of output: 2720
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16793 +/- ##
==========================================
- Coverage 36.44% 36.44% -0.01%
==========================================
Files 6852 6852
Lines 143492 143520 +28
Branches 40952 40960 +8
==========================================
+ Hits 52290 52299 +9
- Misses 91202 91221 +19 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 77 Total Test Services: 0 Failed, 74 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
svanaeinars
added
automerge
oddsson
added
automerge
* Add top level navigation field to organization page model * Add missing slash prefix * Remove sitemap model * Fix export --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Organization page - top level navigation links
What
This change is mostly backend related.
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
OrganizationPagemodel to include new navigation link structures.