chore(web): Move organization subpage content into separate component #16915
Conversation
RunarVestmann
added
the
deprecated:automerge
WalkthroughThe changes in this pull request involve a significant refactor of 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16915 +/- ##
==========================================
- Coverage 36.46% 36.45% -0.01%
==========================================
Files 6903 6903
Lines 144576 144559 -17
Branches 41284 41278 -6
==========================================
- Hits 52714 52700 -14
+ Misses 91862 91859 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![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 (6)
apps/web/screens/Organization/SubPage.tsx (6)
69-69: Redundant namespace hook usageThe variable
nis assigned usinguseNamespace(namespace), but it appears thatnis only used once in theTOCcomponent'stitleprop. Consider usinguseNamespace(namespace)directly within the component where it's needed to reduce unnecessary variable assignments.
Line range hint
98-101: Possible type assertion issues withsubpage.descriptionThe
webRichTextfunction is called withsubpage?.description as SliceType[], which could lead to runtime errors ifsubpage.descriptionis not of typeSliceType[]. Casting withasdoes not perform any runtime checks.Consider adding type guards or validations to ensure
subpage.descriptionis indeed an array ofSliceType.
155-178: Simplify conditional rendering of the sign language videoThe code for rendering the
SignLanguageButtonwithin the conditionsubpage?.signLanguageVideo?.urlis quite nested and complex.Consider extracting the
SignLanguageButtonrendering logic into its own component or simplifying the JSX to improve readability.
Line range hint
343-372: Add TypeScript types to thegetPropsmethod parametersThe
getPropsmethod lacks explicit TypeScript types for its parameters. Adding types improves code readability and helps prevent bugs.Consider updating the method signature:
- SubPage.getProps = async ({ apolloClient, locale, query, req }) => { + SubPage.getProps = async ({ + apolloClient, + locale, + query, + req, + }: GetServerSidePropsContext) => {Ensure you import
GetServerSidePropsContextfromnext.
Line range hint
437-454: Simplify thegetSlugAndSubSlugfunction logicThe
getSlugAndSubSlugfunction can be simplified by using optional chaining and default values.Consider refactoring as follows:
const getSlugAndSubSlug = (query: ParsedUrlQuery, pathname: string) => { const pathSegments = pathname?.split('/') ?? [] let { slug, subSlug } = query - if (!slug && path.length >= 2) { - slug = path[path.length - 2] + slug = slug ?? pathSegments[pathSegments.length - 2] - } - if (!subSlug && path.length > 0) { - subSlug = path.pop() + subSlug = subSlug ?? pathSegments[pathSegments.length - 1] - } return { slug, subSlug } }
Line range hint
486-486: Consider customizing the exported component withNext.jstypesWhen exporting the
SubPagecomponent wrapped withwithMainLayout, consider specifying the component type to ensure proper typing withNext.js.Update the export statement:
-export default withMainLayout(SubPage) +export default withMainLayout(SubPage as NextPage)Ensure
NextPageis imported fromnext.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/screens/Organization/SubPage.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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."
🔇 Additional comments (2)
apps/web/screens/Organization/SubPage.tsx (2)
301-306: Pass only necessary props to SubPageContent
When rendering SubPageContent, you pass subpage, namespace, and organizationPage. Verify that SubPageContent does not require additional props that might be necessary for its internal logic.
283-285: Confirm navigation data adheres to expected structure
Ensure that the navigationData prop provided to OrganizationWrapper matches the expected structure, especially since navList may be empty if organizationPage?.menuLinks is undefined.
Run the following script to verify the structure of navigationData:
✅ Verification successful
Navigation data structure is correctly implemented
Based on the verification:
- The
NavigationDatainterface in OrganizationWrapper expectsitemsto beNavigationItem[] navListis correctly typed asNavigationItem[]and is constructed fromorganizationPage?.menuLinks- Even if
menuLinksis undefined, the optional chaining (?.) will result innavListbeingundefined, which is safely handled by the Navigation component as seen in theNavigationIteminterface whereitemsis optional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that navigationData.items is an array of NavigationItem
# Expected: All items should have 'title', 'href', and optional 'items' properties.
rg 'navigationData\s*=\s*{\s*title:\s*\w+,\s*items:\s*(.*)}' -A5 apps/web/components/OrganizationWrapper.tsx
Length of output: 270
Script:
#!/bin/bash
# Let's first check the OrganizationWrapper component to understand the expected structure
rg -A 5 "interface.*NavigationData" apps/web/components/OrganizationWrapper.tsx
# Then check how navList is constructed
rg -B 3 -A 3 "navList" apps/web/screens/Organization/SubPage.tsx
# Also check for any type definitions related to navigation
ast-grep --pattern 'interface $_ {
$$$
items?: $$$
$$$
}'
Length of output: 3396
Script:
#!/bin/bash
# Let's check the OrganizationWrapper component in the current directory structure
fd -e tsx -e ts OrganizationWrapper
# Let's also check how navigationData is used in the component
rg -B 2 -A 2 "navigationData.*=" apps/web/screens/Organization/SubPage.tsx
# And check the NavigationData type definition
fd -e tsx -e ts -x rg -l "type.*NavigationData|interface.*NavigationData"
Length of output: 513
Script:
#!/bin/bash
# Let's check the NavigationData interface in the OrganizationWrapper
rg -B 2 -A 5 "interface.*NavigationData" apps/web/components/Organization/Wrapper/OrganizationWrapper.tsx
# And check how navList is typed and constructed
rg -B 5 -A 2 "const navList.*=" apps/web/screens/Organization/SubPage.tsx
# Also check the menuLinks type
ast-grep --pattern 'type $_ = {
menuLinks: $$$
}'
Length of output: 738
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
|
kodiakhq
bot
deleted the
chore/web-organization-subpage-content-refactor
branch
…#16915) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Move organization subpage content into separate component
What
Why
Checklist:
Summary by CodeRabbit
New Features
SubPageContentcomponent for improved content rendering.Refactor
SubPagecomponent into distinct layout and content components.