Added Metadata for dynamic pages and static pages #1414
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces new layout and metadata management infrastructure across multiple frontend routes. It adds layout components and dynamic or static metadata generation for pages such as projects, organizations, committees, chapters, members, snapshots, contribute, and about. Metadata is generated using new utility modules that centralize SEO configuration and enforce consistency. The main application layout receives expanded metadata, including Open Graph and Twitter card details. Supporting utilities for Apollo server-side client configuration and CSRF token retrieval are added. Additionally, Jest coverage is updated to exclude all layout files from coverage reports. Minor UI optimization is made in the header for image loading priority. Changes
Suggested reviewers
Suggested labels
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 15
🧹 Nitpick comments (20)
docker/docker-compose-local.yaml (1)
6-6: Consider adding jinja2 to requirements.txt insteadWhile adding the pip install command works, it's better to include this dependency in your requirements.txt file to:
- Make builds more reproducible
- Reduce container startup time
- Follow Docker best practices of keeping containers immutable
Consider moving this dependency to the project's requirements.txt file instead of installing it at runtime.
frontend/src/server/fetchCsrfTokenServer.ts (1)
17-21: Simplify error handling in catch blockThe current error handling logic in the catch block doesn't add any value since it just re-throws the error.
- catch (error) { - if (error) { - throw error - } - } + catch (error) { + throw error + }frontend/src/app/chapters/layout.tsx (1)
2-2: Remove unnecessary React import
With the new JSX transform in Next.js 13+, you no longer need to import React in every.tsxfile. You can safely remove this line to reduce boilerplate.frontend/src/app/contribute/layout.tsx (2)
1-2: Remove unused React import
React is not referenced directly in this file thanks to the automatic JSX runtime. You can remove this import to clean up the code.
4-4: AddMetadatatype annotation for consistency
Other static layout files explicitly type themetadataexport. Consider adding: Metadatahere:-export const metadata = getStaticMetadata('contribute', '/contribute') +export const metadata: Metadata = getStaticMetadata('contribute', '/contribute')frontend/src/app/organizations/layout.tsx (1)
1-2: Remove unnecessary React import
With the new JSX transform, the explicit React import is no longer required.frontend/src/app/members/layout.tsx (1)
1-2: Remove unnecessary React import
The React import can be omitted under the automatic JSX runtime.frontend/src/app/about/layout.tsx (2)
1-2: Remove redundant React import
Under the new JSX runtime, you can drop the React import to clean up the file.
6-8: Consider renaming component toAboutLayout
For consistency with other layouts (ChaptersLayout,ContributeLayout, etc.), you may want to rename this function fromAbouttoAboutLayout.-export default function About({ children }: { children: React.ReactNode }) { +export default function AboutLayout({ children }: { children: React.ReactNode }) {frontend/src/utils/metadata.ts (1)
1-57: Well-structured metadata configuration object.This is a well-organized centralized configuration for page metadata, which follows a consistent structure across all entries. Each page has appropriate descriptions, keywords, titles, and type definitions that will help with SEO.
A few suggestions for further improvement:
- Consider adding more specific keywords for some pages (especially snapshots which has only 2 keywords)
- The snapshots description is quite minimal compared to others and could be expanded
frontend/src/app/organizations/[organizationKey]/layout.tsx (1)
23-27: Remove unnecessary optional chaining and add keywords.Since you've already checked that
organizationexists on line 20, the optional chaining (?.) operators on line 24 are unnecessary. Also, consider adding keywords for consistency with other entity metadata implementations.return generateSeoMetadata({ - title: organization?.name || organization?.login, + title: organization.name || organization.login, description: organization?.description || 'Discover details about this OWASP organization.', canonicalPath: `/organizations/${organizationKey}`, + keywords: ['owasp', 'security', 'organization', organizationKey, organization.name || organization.login], })frontend/src/utils/helpers/apolloClientServer.ts (3)
7-9: Use strict equality comparison.When checking environment variables, it's better to use strict equality comparison (
===) instead of loose equality (==).async function createApolloServerClient() { - const url = (ENVIRONMENT == 'docker') ? GRAPHQL_URL_DOCKER : GRAPHQL_URL + const url = (ENVIRONMENT === 'docker') ? GRAPHQL_URL_DOCKER : GRAPHQL_URL
28-33: Consider adding TypeScript type for Apollo state.The Apollo state restoration lacks proper typing, which might cause type errors.
+// Define the type for the Apollo state +declare global { + var __APOLLO_STATE__: any; +} return new ApolloClient({ cache: new InMemoryCache().restore(globalThis.__APOLLO_STATE__ || {}), link: authLink.concat(httpLink), ssrMode: true, })
35-35: Consider lazy initialization for better testability.The Apollo client is initialized during module evaluation, which could make testing more difficult. Consider using a lazy initialization pattern.
-export const apolloServerClient = await createApolloServerClient() +let _apolloServerClient: ApolloClient<any> | null = null +export const apolloServerClient = async () => { + if (!_apolloServerClient) { + _apolloServerClient = await createApolloServerClient() + } + return _apolloServerClient +}This change would require updating all usage sites to call the function instead of using the value directly.
frontend/src/app/snapshots/[id]/layout.tsx (2)
22-24: Remove unnecessary optional chaining.The code already checks for the existence of
snapshoton line 18, so the optional chaining onsnapshot?.titleis unnecessary.return generateSeoMetadata({ - title: snapshot?.title, - description: `Discover details about ${snapshot?.title} OWASP snapshot.`, + title: snapshot.title, + description: `Discover details about ${snapshot.title} OWASP snapshot.`, canonicalPath: `/snapshots/${snapshotKey}`, keywords: ['owasp', 'snapshot', snapshotKey, snapshot?.title], })
24-25: Remove optional chaining from keywords array.Since we've already verified
snapshotexists, the optional chaining in the keywords array is unnecessary.canonicalPath: `/snapshots/${snapshotKey}`, - keywords: ['owasp', 'snapshot', snapshotKey, snapshot?.title], + keywords: ['owasp', 'snapshot', snapshotKey, snapshot.title],frontend/src/app/members/[memberKey]/layout.tsx (1)
27-27: Filter out undefined values from keywords array.The keywords array might include undefined values if
user.nameis null or undefined, which could lead to unexpected metadata values.canonicalPath: `/members/${memberKey}`, - keywords: [user.login, user.name, 'owasp', 'owasp community member'], + keywords: [user.login, user.name, 'owasp', 'owasp community member'].filter(Boolean),frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/layout.tsx (1)
18-18: Consider object property shorthand for cleaner code.When variable names match property names, you can use object property shorthand for cleaner code.
- variables: { repositoryKey: repositoryKey, organizationKey: organizationKey }, + variables: { repositoryKey, organizationKey },frontend/src/utils/metaconfig.ts (2)
72-86: Great implementation of static metadata generation.The
getStaticMetadatafunction provides a good abstraction for generating metadata for static pages. The error handling for missing configurations is a nice touch.Consider adding type validation for the expected properties in
METADATA_CONFIG[pageKey].export function getStaticMetadata(pageKey, canonicalPath?: string): Metadata { if (!METADATA_CONFIG[pageKey]) { throw new Error(`No metadata configuration found for key: ${pageKey}`) } const config = METADATA_CONFIG[pageKey] + // Validate required properties + if (!config.pageTitle || !config.description) { + throw new Error(`Incomplete metadata configuration for key: ${pageKey}`) + } return generateSeoMetadata({ title: config.pageTitle, description: config.description, keywords: config.keywords, type: config.type, canonicalPath: canonicalPath || `/${pageKey.toLowerCase()}`, }) }
1-87: Add JSDoc comments for better code documentation.Consider adding JSDoc comments to the functions to explain their purpose, parameters, and return values. This would make the code more maintainable and easier to understand for new contributors.
import { Metadata } from 'next' import { METADATA_CONFIG } from './metadata' interface SeoMetadataProps { title: string description: string canonicalPath?: string ogImage?: string noIndex?: boolean keywords?: string[] type?: 'website' | 'article' | 'profile' locale?: string siteName?: string twitterHandle?: string } +/** + * Generates SEO metadata for Next.js pages + * + * @param {SeoMetadataProps} props - SEO metadata properties + * @returns {Metadata} - Next.js Metadata object + */ export function generateSeoMetadata({ title, description, canonicalPath, ogImage = 'https://nest.owasp.org/img/owasp_icon_white_background.png', noIndex = false, keywords = [], type = 'website', locale = 'en_US', siteName = 'OWASP Nest', twitterHandle = '@owasp', }: SeoMetadataProps): Metadata { // Function implementation... } +/** + * Retrieves and generates metadata for static pages based on predefined configurations + * + * @param {string} pageKey - The key identifying the page in METADATA_CONFIG + * @param {string} [canonicalPath] - Optional override for the canonical path + * @returns {Metadata} - Next.js Metadata object + * @throws {Error} - If no metadata configuration is found for the given key + */ export function getStaticMetadata(pageKey, canonicalPath?: string): Metadata { // Function implementation... }🧰 Tools
🪛 GitHub Actions: Run CI/CD
[error] 22-22: ESLint: 'noIndex' is assigned a value but never used. Allowed unused args must match /^_/u (@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
docker/docker-compose-local.yaml(1 hunks)frontend/.env.example(1 hunks)frontend/src/app/about/layout.tsx(1 hunks)frontend/src/app/chapters/[chapterKey]/layout.tsx(1 hunks)frontend/src/app/chapters/layout.tsx(1 hunks)frontend/src/app/committees/[committeeKey]/layout.tsx(1 hunks)frontend/src/app/committees/layout.tsx(1 hunks)frontend/src/app/contribute/layout.tsx(1 hunks)frontend/src/app/layout.tsx(1 hunks)frontend/src/app/members/[memberKey]/layout.tsx(1 hunks)frontend/src/app/members/layout.tsx(1 hunks)frontend/src/app/organizations/[organizationKey]/layout.tsx(1 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/layout.tsx(1 hunks)frontend/src/app/organizations/layout.tsx(1 hunks)frontend/src/app/projects/[projectKey]/layout.tsx(1 hunks)frontend/src/app/projects/layout.tsx(1 hunks)frontend/src/app/snapshots/[id]/layout.tsx(1 hunks)frontend/src/app/snapshots/layout.tsx(1 hunks)frontend/src/components/Header.tsx(1 hunks)frontend/src/server/fetchCsrfTokenServer.ts(1 hunks)frontend/src/utils/credentials.ts(1 hunks)frontend/src/utils/helpers/apolloClientServer.ts(1 hunks)frontend/src/utils/metaconfig.ts(1 hunks)frontend/src/utils/metadata.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
frontend/src/app/snapshots/layout.tsx (9)
frontend/src/app/chapters/layout.tsx (1)
metadata(5-5)frontend/src/app/about/layout.tsx (1)
metadata(5-5)frontend/src/app/committees/layout.tsx (1)
metadata(5-5)frontend/src/app/contribute/layout.tsx (1)
metadata(4-4)frontend/src/app/layout.tsx (1)
metadata(22-57)frontend/src/app/members/layout.tsx (1)
metadata(5-5)frontend/src/app/organizations/layout.tsx (1)
metadata(5-5)frontend/src/app/projects/layout.tsx (1)
metadata(5-5)frontend/src/utils/metaconfig.ts (1)
getStaticMetadata(72-86)
frontend/src/app/chapters/layout.tsx (9)
frontend/src/app/about/layout.tsx (1)
metadata(5-5)frontend/src/app/committees/layout.tsx (1)
metadata(5-5)frontend/src/app/contribute/layout.tsx (1)
metadata(4-4)frontend/src/app/layout.tsx (1)
metadata(22-57)frontend/src/app/members/layout.tsx (1)
metadata(5-5)frontend/src/app/organizations/layout.tsx (1)
metadata(5-5)frontend/src/app/projects/layout.tsx (1)
metadata(5-5)frontend/src/app/snapshots/layout.tsx (1)
metadata(4-4)frontend/src/utils/metaconfig.ts (1)
getStaticMetadata(72-86)
frontend/src/app/committees/[committeeKey]/layout.tsx (3)
frontend/src/utils/helpers/apolloClientServer.ts (1)
apolloServerClient(35-35)frontend/src/server/queries/committeeQueries.ts (1)
GET_COMMITTEE_DATA(3-26)frontend/src/utils/metaconfig.ts (1)
generateSeoMetadata(17-70)
frontend/src/server/fetchCsrfTokenServer.ts (1)
frontend/src/utils/credentials.ts (3)
ENVIRONMENT(4-4)CSRF_URL(2-2)CSRF_URL_DOCKER(3-3)
frontend/src/app/about/layout.tsx (9)
frontend/src/app/chapters/layout.tsx (1)
metadata(5-5)frontend/src/app/committees/layout.tsx (1)
metadata(5-5)frontend/src/app/contribute/layout.tsx (1)
metadata(4-4)frontend/src/app/layout.tsx (1)
metadata(22-57)frontend/src/app/members/layout.tsx (1)
metadata(5-5)frontend/src/app/organizations/layout.tsx (1)
metadata(5-5)frontend/src/app/projects/layout.tsx (1)
metadata(5-5)frontend/src/app/snapshots/layout.tsx (1)
metadata(4-4)frontend/src/utils/metaconfig.ts (1)
getStaticMetadata(72-86)
frontend/src/app/contribute/layout.tsx (9)
frontend/src/app/chapters/layout.tsx (1)
metadata(5-5)frontend/src/app/about/layout.tsx (1)
metadata(5-5)frontend/src/app/committees/layout.tsx (1)
metadata(5-5)frontend/src/app/layout.tsx (1)
metadata(22-57)frontend/src/app/members/layout.tsx (1)
metadata(5-5)frontend/src/app/organizations/layout.tsx (1)
metadata(5-5)frontend/src/app/projects/layout.tsx (1)
metadata(5-5)frontend/src/app/snapshots/layout.tsx (1)
metadata(4-4)frontend/src/utils/metaconfig.ts (1)
getStaticMetadata(72-86)
frontend/src/app/members/layout.tsx (9)
frontend/src/app/chapters/layout.tsx (1)
metadata(5-5)frontend/src/app/about/layout.tsx (1)
metadata(5-5)frontend/src/app/committees/layout.tsx (1)
metadata(5-5)frontend/src/app/contribute/layout.tsx (1)
metadata(4-4)frontend/src/app/layout.tsx (1)
metadata(22-57)frontend/src/app/organizations/layout.tsx (1)
metadata(5-5)frontend/src/app/projects/layout.tsx (1)
metadata(5-5)frontend/src/app/snapshots/layout.tsx (1)
metadata(4-4)frontend/src/utils/metaconfig.ts (1)
getStaticMetadata(72-86)
frontend/src/app/snapshots/[id]/layout.tsx (9)
frontend/src/app/chapters/[chapterKey]/layout.tsx (1)
generateMetadata(9-27)frontend/src/app/committees/[committeeKey]/layout.tsx (1)
generateMetadata(7-29)frontend/src/app/organizations/[organizationKey]/layout.tsx (1)
generateMetadata(7-28)frontend/src/app/members/[memberKey]/layout.tsx (1)
generateMetadata(7-29)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/layout.tsx (1)
generateMetadata(7-30)frontend/src/app/projects/[projectKey]/layout.tsx (1)
generateMetadata(7-31)frontend/src/utils/helpers/apolloClientServer.ts (1)
apolloServerClient(35-35)frontend/src/server/queries/snapshotQueries.ts (1)
GET_SNAPSHOT_DETAILS(3-57)frontend/src/utils/metaconfig.ts (1)
generateSeoMetadata(17-70)
frontend/src/app/members/[memberKey]/layout.tsx (4)
frontend/src/utils/helpers/apolloClientServer.ts (1)
apolloServerClient(35-35)frontend/src/server/queries/userQueries.ts (1)
GET_USER_DATA(3-59)frontend/src/types/user.ts (1)
user(3-18)frontend/src/utils/metaconfig.ts (1)
generateSeoMetadata(17-70)
🪛 GitHub Actions: Run CI/CD
frontend/src/utils/metaconfig.ts
[error] 22-22: ESLint: 'noIndex' is assigned a value but never used. Allowed unused args must match /^_/u (@typescript-eslint/no-unused-vars)
🔇 Additional comments (27)
frontend/src/utils/credentials.ts (1)
3-3: LGTM - Docker-specific environment variables addedThese new environment variables support server-side operations in Docker environments, which aligns with the metadata infrastructure updates in this PR.
Also applies to: 6-6
frontend/src/components/Header.tsx (1)
61-61: Good UX optimization for logo imagesAdding
priority={true}to these OWASP logo images is a good practice that will help improve the Largest Contentful Paint (LCP) metrics by prioritizing the loading of these critical UI elements.Also applies to: 69-69
frontend/src/app/chapters/layout.tsx (3)
3-3: Verify module path alias
Ensure that yourtsconfig.json(or equivalent) defines a path alias forutils/*so thatimport { getStaticMetadata } from 'utils/metaconfig'resolves correctly.
5-5: Static metadata export is correctly implemented
getStaticMetadata('chapters', '/chapters')follows the established pattern for static pages and will throw an error if the key is missing inMETADATA_CONFIG.
7-9: Layout component correctly renders children
Returningchildrendirectly is sufficient for this simple route wrapper. No additional markup or logic is needed here.frontend/src/app/contribute/layout.tsx (1)
6-8: Layout component implementation is correct
ContributeLayoutsimply renders itschildren, matching the conventions used elsewhere.frontend/src/app/organizations/layout.tsx (2)
5-5: Static metadata export aligns with pattern
UsinggetStaticMetadata('organizations', '/organizations')matches the other top‑level routes.
7-9: Ensure CSS class name matches existing styles
You’ve wrappedchildrenin<div className="organizations-layout">. Verify that the corresponding CSS/SCSS for.organizations-layoutexists, otherwise the wrapper may have no effect.frontend/src/app/members/layout.tsx (1)
5-5: Metadata export is correctly configured
getStaticMetadata('members', '/members')will pull the right config fromMETADATA_CONFIG.frontend/src/app/about/layout.tsx (1)
5-5: Static metadata export is correct
getStaticMetadata('about', '/about')matches the pattern used for other static pages and will throw if the key is missing.frontend/src/app/committees/layout.tsx (1)
1-9: Well structured metadata implementation for committees route.Good implementation of a layout component that follows the established pattern in the application. The metadata is appropriately generated using the centralized utility function, and the layout component is kept minimal, returning only its children.
frontend/src/app/snapshots/layout.tsx (1)
1-8: Properly implemented layout component for snapshots route.This implementation correctly follows the established pattern in the application for route-specific layouts and metadata. The component is simple and focused, and the metadata is generated using the centralized utility.
frontend/src/app/layout.tsx (1)
22-57: Comprehensive SEO metadata implementation for the home page.The enhanced metadata includes all essential SEO elements:
- Appropriate title and description
- Complete Open Graph properties with image dimensions
- Twitter card metadata with appropriate card type
- Consistent branding across all metadata properties
This implementation follows SEO best practices and aligns with the metadata strategy used throughout the application.
frontend/src/app/projects/layout.tsx (1)
1-9: Well-implemented layout component for projects route.This implementation correctly follows the established pattern in the application, using the centralized metadata utility and creating a simple layout component that returns its children.
frontend/src/app/chapters/[chapterKey]/layout.tsx (2)
1-8: Good organization of imports and type declarations.The imports and type declarations are well-organized. The explicit type declaration for
Paramsimproves readability and maintainability.
29-31: Good implementation of the layout component.The layout component is simple and follows the React functional component pattern well, correctly typing the children prop.
frontend/src/app/organizations/[organizationKey]/layout.tsx (2)
1-11: Good organization of imports and type declarations.The imports are well-organized and the parameter type for the
generateMetadatafunction is correctly defined.
30-32: Good implementation of the layout component.The layout component is well-structured and follows the React functional component pattern correctly.
frontend/src/app/committees/[committeeKey]/layout.tsx (3)
1-11: Good organization of imports and type declarations.The imports are well-organized and the parameter type for the
generateMetadatafunction is correctly defined.
23-28: Good implementation of SEO metadata.The SEO metadata implementation is good with appropriate title, description, canonical path, and keywords.
31-33: Good implementation of the layout component.The layout component is well-structured and follows the React functional component pattern correctly.
frontend/src/utils/helpers/apolloClientServer.ts (2)
1-6: Good organization of imports.The imports are well-organized, grouping Apollo Client, Next.js, and application-specific utilities together.
37-44: Good implementation of CSRF token retrieval.The CSRF token retrieval function is well-implemented, checking for an existing token in cookies before making a request to fetch a new one.
frontend/src/app/projects/[projectKey]/layout.tsx (1)
33-35:❓ Verification inconclusive
Consider maintaining layout consistency across components.
Some layout components (like
UserDetailsLayout) wrap children in a div with a class, while others (like this one) simply return children. Consider whether you need a wrapper div with styling here for consistency.
🏁 Script executed:
#!/bin/bash # Check how other layout components are implemented echo "Checking layout components implementation patterns..." fd --type f "layout.tsx" src/app -x grep -l "return children" {} \; echo "-----" fd --type f "layout.tsx" src/app -x grep -l "className=" {} \;Length of output: 460
🏁 Script executed:
#!/bin/bash echo "Found layout.tsx files:" fd --type f layout.tsx frontend/src/app echo echo "Layouts returning children only:" fd --type f layout.tsx frontend/src/app -x grep -H "return children" {} \; echo echo "Layouts with wrapper div or className:" fd --type f layout.tsx frontend/src/app -x grep -H "className=" {} \;Length of output: 2489
Assess layout wrapper consistency across pages
We’ve observed two patterns in
layout.tsxcomponents underfrontend/src/app:
Some wrap
childrenin a<div className="…-layout">for styling
• members/layout.tsx →users-layout
• members/[memberKey]/layout.tsx →user-details-layout
• organizations/layout.tsx →organizations-layout
• organizations/[organizationKey]/layout.tsx →organization-details-layout
• chapters/[chapterKey]/layout.tsx →chapter-layout
• committees/[committeeKey]/layout.tsx →committee-layoutOthers simply
return childrenwith no wrapper or className
• projects/layout.tsx & projects/[projectKey]/layout.tsx
• chapters/layout.tsx
• committees/layout.tsx
• snapshots/layout.tsx & snapshots/[id]/layout.tsx
• about/layout.tsx
• contribute/layout.tsx
• organizations/[organizationKey]/repositories/[repositoryKey]/layout.tsxPlease decide if
ProjectDetailsLayout(and/or its parentprojects/layout.tsx) requires a wrapper<div>with a classname (e.g.project-layoutorproject-details-layout) to align with your global styling convention, or if omitting wrappers in these pages is intentional.frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/layout.tsx (1)
1-35: Overall design pattern is consistent with other layout files.The implementation follows the same pattern used across other entity pages, with appropriate metadata generation based on dynamic route parameters. The consistency in approach is good for maintainability.
frontend/src/utils/metaconfig.ts (2)
4-15: Great job defining a comprehensive SEO metadata interface!The
SeoMetadataPropsinterface is well-structured and covers the essential metadata properties needed for SEO. The optional properties are appropriately marked with the?operator.
35-69: LGTM - Well-structured metadata object!The metadata object is well-structured and follows Next.js metadata API conventions. Good use of conditional spreading to include optional properties only when needed.
frontend/.env.example
Outdated
| NEXT_PUBLIC_CSRF_URL_DOCKER=http://nest-backend:8000/csrf/ | ||
| NEXT_PUBLIC_ENVIRONMENT=docker |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Docker-specific URLs and environment change
The addition of Docker-specific URLs and changing the default environment to 'docker' supports the server-side rendering infrastructure, but this could affect non-Docker development setups.
Also applies to: 6-6
🌐 Web query:
What is the standard practice for managing multiple environment configurations in Next.js applications?
💡 Result:
Standard Practice for Managing Multiple Environment Configurations in Next.js
Managing multiple environment configurations is critical in Next.js applications to ensure security, proper functionality, and ease of deployment across development, testing, staging, and production. The Next.js ecosystem provides a clear, standardized approach for handling this.
Environment-Specific .env Files
- Utilize separate environment variable files for each environment:
.envor.env.localfor local development.env.developmentfor development.env.testfor testing.env.productionfor production
- Next.js automatically loads the appropriate file based on the current
NODE_ENV(e.g., settingNODE_ENV=productionloads.env.production) [4][5][10]. - Sensitive variables should be stored in files that are excluded from version control (such as
.env.local), while placeholders or defaults can be provided in.env.example[8].
Public vs Private Environment Variables
- Variables that need to be accessed in client-side code must be prefixed with
NEXT_PUBLIC_(e.g.,NEXT_PUBLIC_API_URL). These are exposed to both server and client [7][8][10]. - Variables without this prefix are only available on the server side and should be used for sensitive data such as API secrets [7][8].
Accessing Variables in Code
- Access your environment variables using
process.env, for example:const apiUrl = process.env.NEXT_PUBLIC_API_URL;
- Use fallback/default values to handle missing variables and prevent runtime errors:
const siteTitle = process.env.NEXT_PUBLIC_SITE_TITLE || 'Default Site Title';
Dynamic Configurations with next.config.js
- For more advanced scenarios, use
next.config.jsfor programmatic configuration:- Modify or inject variables based on logic, such as detecting the current phase/environment [3][4].
- Use the configuration function pattern, leveraging
phasefromnext/constantsto target specific environments:const { PHASE_DEVELOPMENT_SERVER } = require('next/constants'); module.exports = (phase) => { if (phase === PHASE_DEVELOPMENT_SERVER) { return { // Dev-specific config }; } return { // Prod or other configs }; };
- Environment variables configured in
next.config.jscan also be exposed to the browser if set in theenvfield or by using Webpack'sDefinePlugin[2][3][4].
Scripts for Clear Separation
- Define scripts in
package.jsonto help visually and operationally separate environment commands:This avoids mistakes like running a build with the wrong environment settings [4]."scripts": { "dev": "next dev", "build": "next build", "start": "next start", "test": "NODE_ENV=test next build" }
Best Practices Summary
- Never commit sensitive environment files (like
.env.local) to version control. - Use
NEXT_PUBLIC_only for variables safe to expose to the browser. - Keep variable naming consistent and descriptive, avoiding special characters [6].
- Use environment files for static variables, and consider runtime configuration solutions for variables that need to change without rebuilding the app [6].
By following these practices, teams can confidently manage environment-specific configurations in Next.js, enabling safer, more predictable, and easier-to-maintain deployments across multiple environments [4][5][10].
Citations:
- 1: https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables
- 2: https://nextjs.org/docs/app/api-reference/config/next-config-js/env
- 3: https://nextjs.org/docs/app/api-reference/config/next-config-js
- 4: https://www.dhiwise.com/post/how-to-manage-nextjs-environment-variables-for-better-security
- 5: https://www.geeksforgeeks.org/how-to-handle-different-environments-in-a-next-js/
- 6: https://nextjsstarter.com/blog/nextjs-environment-variables-secure-management-guide/
- 7: https://www.mdfaisal.com/blog/how-to-handle-different-environments-in-a-nextjs-application
- 8: https://www.dhiwise.com/post/how-to-configure-environment-variables-in-nextjs
- 9: Best way to handle .env variables vercel/next.js#33223
- 10: https://blog.devops.dev/managing-environment-variables-across-different-environments-in-next-js-using-docker-dfdab1a60290
Align environment configurations with Next.js recommended .env conventions
To avoid impacting non‑Docker development and follow Next.js best practices:
- Remove Docker‑specific defaults from
frontend/.env.exampleso it remains a generic template of all required keys. - Create dedicated files for each environment:
.env.local(ignored in VCS) for your local overrides.env.developmentfor standard development (non‑Docker).env.docker(or extend your Docker Compose) to holdNEXT_PUBLIC_CSRF_URL_DOCKERandNEXT_PUBLIC_ENVIRONMENT=docker.env.productionfor production defaults
- Use
NODE_ENVor a custom script to load the correct .env file. For example inpackage.json:"scripts": { "dev": "next dev", "dev:docker": "cp .env.docker .env.local && next dev", "build": "next build", "start": "next start" }
- Document in your README how to start the app in each environment and which .env files to use.
By splitting configs this way, you keep .env.example environment‑agnostic and ensure developers only pick up Docker settings when explicitly intended.
frontend/src/app/members/layout.tsx
Outdated
| export default function UsersLayout({ children }: { children: React.ReactNode }) { | ||
| return <div className="users-layout">{children}</div> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename UsersLayout to MembersLayout for consistency
Since this file lives under /members, the component and CSS class should reflect that. For example:
-export default function UsersLayout({ children }: { children: React.ReactNode }) {
- return <div className="users-layout">{children}</div>
-}
+export default function MembersLayout({ children }: { children: React.ReactNode }) {
+ return <div className="members-layout">{children}</div>
+}This will keep naming aligned across the codebase.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function UsersLayout({ children }: { children: React.ReactNode }) { | |
| return <div className="users-layout">{children}</div> | |
| } | |
| export default function MembersLayout({ children }: { children: React.ReactNode }) { | |
| return <div className="members-layout">{children}</div> | |
| } |
| export async function generateMetadata({ | ||
| params, | ||
| }: { | ||
| params: Promise<{ memberKey: string }> | ||
| }): Promise<Metadata> { | ||
| const { memberKey } = await params | ||
| const { data } = await apolloServerClient.query({ | ||
| query: GET_USER_DATA, | ||
| variables: { | ||
| key: memberKey, | ||
| }, | ||
| }) | ||
| const user = data?.user | ||
| if (!user) { | ||
| return | ||
| } | ||
| return generateSeoMetadata({ | ||
| title: user.name || user.login, | ||
| description: user.bio || 'Discover details about this OWASP community member.', | ||
| canonicalPath: `/members/${memberKey}`, | ||
| keywords: [user.login, user.name, 'owasp', 'owasp community member'], | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the GraphQL query.
The generateMetadata function doesn't handle potential errors from the GraphQL query. Add try/catch to prevent page failures if the query fails.
export async function generateMetadata({
params,
}: {
params: Promise<{ memberKey: string }>
}): Promise<Metadata> {
const { memberKey } = await params
- const { data } = await apolloServerClient.query({
- query: GET_USER_DATA,
- variables: {
- key: memberKey,
- },
- })
- const user = data?.user
- if (!user) {
- return
- }
- return generateSeoMetadata({
- title: user.name || user.login,
- description: user.bio || 'Discover details about this OWASP community member.',
- canonicalPath: `/members/${memberKey}`,
- keywords: [user.login, user.name, 'owasp', 'owasp community member'],
- })
+ try {
+ const { data } = await apolloServerClient.query({
+ query: GET_USER_DATA,
+ variables: {
+ key: memberKey,
+ },
+ })
+ const user = data?.user
+ if (!user) {
+ return
+ }
+ return generateSeoMetadata({
+ title: user.name || user.login,
+ description: user.bio || 'Discover details about this OWASP community member.',
+ canonicalPath: `/members/${memberKey}`,
+ keywords: [user.login, user.name, 'owasp', 'owasp community member'],
+ })
+ } catch (error) {
+ console.error('Error fetching user metadata:', error)
+ return
+ }
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function generateMetadata({ | |
| params, | |
| }: { | |
| params: Promise<{ memberKey: string }> | |
| }): Promise<Metadata> { | |
| const { memberKey } = await params | |
| const { data } = await apolloServerClient.query({ | |
| query: GET_USER_DATA, | |
| variables: { | |
| key: memberKey, | |
| }, | |
| }) | |
| const user = data?.user | |
| if (!user) { | |
| return | |
| } | |
| return generateSeoMetadata({ | |
| title: user.name || user.login, | |
| description: user.bio || 'Discover details about this OWASP community member.', | |
| canonicalPath: `/members/${memberKey}`, | |
| keywords: [user.login, user.name, 'owasp', 'owasp community member'], | |
| }) | |
| } | |
| export async function generateMetadata({ | |
| params, | |
| }: { | |
| params: Promise<{ memberKey: string }> | |
| }): Promise<Metadata> { | |
| const { memberKey } = await params | |
| try { | |
| const { data } = await apolloServerClient.query({ | |
| query: GET_USER_DATA, | |
| variables: { | |
| key: memberKey, | |
| }, | |
| }) | |
| const user = data?.user | |
| if (!user) { | |
| return | |
| } | |
| return generateSeoMetadata({ | |
| title: user.name || user.login, | |
| description: user.bio || 'Discover details about this OWASP community member.', | |
| canonicalPath: `/members/${memberKey}`, | |
| keywords: [user.login, user.name, 'owasp', 'owasp community member'], | |
| }) | |
| } catch (error) { | |
| console.error('Error fetching user metadata:', error) | |
| return | |
| } | |
| } |
| export async function generateMetadata({ | ||
| params, | ||
| }: { | ||
| params: Promise<{ | ||
| projectKey: string | ||
| }> | ||
| }): Promise<Metadata> { | ||
| const { projectKey } = await params | ||
| const { data } = await apolloServerClient.query({ | ||
| query: GET_PROJECT_DATA, | ||
| variables: { | ||
| key: projectKey, | ||
| }, | ||
| }) | ||
| const project = data?.project | ||
| if (!project) { | ||
| return | ||
| } | ||
| return generateSeoMetadata({ | ||
| title: project.name, | ||
| description: project.summary || 'Discover details about this OWASP project.', | ||
| canonicalPath: `/projects/${projectKey}`, | ||
| keywords: ['owasp', 'project', projectKey, project.name], | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the GraphQL query.
Similar to other layout files, add error handling to the GraphQL query to prevent page failures if the query fails.
export async function generateMetadata({
params,
}: {
params: Promise<{
projectKey: string
}>
}): Promise<Metadata> {
const { projectKey } = await params
- const { data } = await apolloServerClient.query({
- query: GET_PROJECT_DATA,
- variables: {
- key: projectKey,
- },
- })
- const project = data?.project
- if (!project) {
- return
- }
- return generateSeoMetadata({
- title: project.name,
- description: project.summary || 'Discover details about this OWASP project.',
- canonicalPath: `/projects/${projectKey}`,
- keywords: ['owasp', 'project', projectKey, project.name],
- })
+ try {
+ const { data } = await apolloServerClient.query({
+ query: GET_PROJECT_DATA,
+ variables: {
+ key: projectKey,
+ },
+ })
+ const project = data?.project
+ if (!project) {
+ return
+ }
+ return generateSeoMetadata({
+ title: project.name,
+ description: project.summary || 'Discover details about this OWASP project.',
+ canonicalPath: `/projects/${projectKey}`,
+ keywords: ['owasp', 'project', projectKey, project.name],
+ })
+ } catch (error) {
+ console.error('Error fetching project metadata:', error)
+ return
+ }
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function generateMetadata({ | |
| params, | |
| }: { | |
| params: Promise<{ | |
| projectKey: string | |
| }> | |
| }): Promise<Metadata> { | |
| const { projectKey } = await params | |
| const { data } = await apolloServerClient.query({ | |
| query: GET_PROJECT_DATA, | |
| variables: { | |
| key: projectKey, | |
| }, | |
| }) | |
| const project = data?.project | |
| if (!project) { | |
| return | |
| } | |
| return generateSeoMetadata({ | |
| title: project.name, | |
| description: project.summary || 'Discover details about this OWASP project.', | |
| canonicalPath: `/projects/${projectKey}`, | |
| keywords: ['owasp', 'project', projectKey, project.name], | |
| }) | |
| } | |
| export async function generateMetadata({ | |
| params, | |
| }: { | |
| params: Promise<{ | |
| projectKey: string | |
| }> | |
| }): Promise<Metadata> { | |
| const { projectKey } = await params | |
| try { | |
| const { data } = await apolloServerClient.query({ | |
| query: GET_PROJECT_DATA, | |
| variables: { | |
| key: projectKey, | |
| }, | |
| }) | |
| const project = data?.project | |
| if (!project) { | |
| return | |
| } | |
| return generateSeoMetadata({ | |
| title: project.name, | |
| description: project.summary || 'Discover details about this OWASP project.', | |
| canonicalPath: `/projects/${projectKey}`, | |
| keywords: ['owasp', 'project', projectKey, project.name], | |
| }) | |
| } catch (error) { | |
| console.error('Error fetching project metadata:', error) | |
| return | |
| } | |
| } |
| export async function generateMetadata({ | ||
| params, | ||
| }: { | ||
| params: Promise<{ | ||
| repositoryKey: string | ||
| organizationKey: string | ||
| }> | ||
| }): Promise<Metadata> { | ||
| const { repositoryKey, organizationKey } = await params | ||
| const { data } = await apolloServerClient.query({ | ||
| query: GET_REPOSITORY_DATA, | ||
| variables: { repositoryKey: repositoryKey, organizationKey: organizationKey }, | ||
| }) | ||
| const repository = data?.repository | ||
| if (!repository) { | ||
| return | ||
| } | ||
| return generateSeoMetadata({ | ||
| title: repository.name, | ||
| description: repository.description || 'Discover details about this OWASP repository.', | ||
| canonicalPath: `/organizations/${organizationKey}/repositories/${repositoryKey}`, | ||
| keywords: ['owasp', 'repository', repositoryKey, repository.name], | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the GraphQL query.
Like the other layout files, add error handling to the GraphQL query to prevent page failures if the query fails.
export async function generateMetadata({
params,
}: {
params: Promise<{
repositoryKey: string
organizationKey: string
}>
}): Promise<Metadata> {
const { repositoryKey, organizationKey } = await params
- const { data } = await apolloServerClient.query({
- query: GET_REPOSITORY_DATA,
- variables: { repositoryKey: repositoryKey, organizationKey: organizationKey },
- })
- const repository = data?.repository
- if (!repository) {
- return
- }
- return generateSeoMetadata({
- title: repository.name,
- description: repository.description || 'Discover details about this OWASP repository.',
- canonicalPath: `/organizations/${organizationKey}/repositories/${repositoryKey}`,
- keywords: ['owasp', 'repository', repositoryKey, repository.name],
- })
+ try {
+ const { data } = await apolloServerClient.query({
+ query: GET_REPOSITORY_DATA,
+ variables: { repositoryKey: repositoryKey, organizationKey: organizationKey },
+ })
+ const repository = data?.repository
+ if (!repository) {
+ return
+ }
+ return generateSeoMetadata({
+ title: repository.name,
+ description: repository.description || 'Discover details about this OWASP repository.',
+ canonicalPath: `/organizations/${organizationKey}/repositories/${repositoryKey}`,
+ keywords: ['owasp', 'repository', repositoryKey, repository.name],
+ })
+ } catch (error) {
+ console.error('Error fetching repository metadata:', error)
+ return
+ }
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function generateMetadata({ | |
| params, | |
| }: { | |
| params: Promise<{ | |
| repositoryKey: string | |
| organizationKey: string | |
| }> | |
| }): Promise<Metadata> { | |
| const { repositoryKey, organizationKey } = await params | |
| const { data } = await apolloServerClient.query({ | |
| query: GET_REPOSITORY_DATA, | |
| variables: { repositoryKey: repositoryKey, organizationKey: organizationKey }, | |
| }) | |
| const repository = data?.repository | |
| if (!repository) { | |
| return | |
| } | |
| return generateSeoMetadata({ | |
| title: repository.name, | |
| description: repository.description || 'Discover details about this OWASP repository.', | |
| canonicalPath: `/organizations/${organizationKey}/repositories/${repositoryKey}`, | |
| keywords: ['owasp', 'repository', repositoryKey, repository.name], | |
| }) | |
| } | |
| export async function generateMetadata({ | |
| params, | |
| }: { | |
| params: Promise<{ | |
| repositoryKey: string | |
| organizationKey: string | |
| }> | |
| }): Promise<Metadata> { | |
| const { repositoryKey, organizationKey } = await params | |
| try { | |
| const { data } = await apolloServerClient.query({ | |
| query: GET_REPOSITORY_DATA, | |
| variables: { repositoryKey: repositoryKey, organizationKey: organizationKey }, | |
| }) | |
| const repository = data?.repository | |
| if (!repository) { | |
| return | |
| } | |
| return generateSeoMetadata({ | |
| title: repository.name, | |
| description: repository.description || 'Discover details about this OWASP repository.', | |
| canonicalPath: `/organizations/${organizationKey}/repositories/${repositoryKey}`, | |
| keywords: ['owasp', 'repository', repositoryKey, repository.name], | |
| }) | |
| } catch (error) { | |
| console.error('Error fetching repository metadata:', error) | |
| return | |
| } | |
| } |
frontend/src/utils/metaconfig.ts
Outdated
| const baseUrl = process.env.NEXT_PUBLIC_BASE_URL || 'https://nest.owasp.org' | ||
|
|
||
| const formattedTitle = title.includes('OWASP Nest') ? title : `${title} – OWASP Nest` | ||
|
|
||
| const canonicalUrl = canonicalPath ? `${baseUrl}${canonicalPath}` : undefined | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure canonicalPath always starts with a forward slash.
The current implementation could create malformed URLs if canonicalPath doesn't start with a /. Consider adding validation or normalization.
const baseUrl = process.env.NEXT_PUBLIC_BASE_URL || 'https://nest.owasp.org'
const formattedTitle = title.includes('OWASP Nest') ? title : `${title} – OWASP Nest`
- const canonicalUrl = canonicalPath ? `${baseUrl}${canonicalPath}` : undefined
+ const canonicalUrl = canonicalPath
+ ? `${baseUrl}${canonicalPath.startsWith('/') ? canonicalPath : `/${canonicalPath}`}`
+ : undefined📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const baseUrl = process.env.NEXT_PUBLIC_BASE_URL || 'https://nest.owasp.org' | |
| const formattedTitle = title.includes('OWASP Nest') ? title : `${title} – OWASP Nest` | |
| const canonicalUrl = canonicalPath ? `${baseUrl}${canonicalPath}` : undefined | |
| const baseUrl = process.env.NEXT_PUBLIC_BASE_URL || 'https://nest.owasp.org' | |
| const formattedTitle = title.includes('OWASP Nest') ? title : `${title} – OWASP Nest` | |
| const canonicalUrl = canonicalPath | |
| ? `${baseUrl}${canonicalPath.startsWith('/') ? canonicalPath : `/${canonicalPath}`}` | |
| : undefined |
frontend/src/utils/metaconfig.ts
Outdated
| export function generateSeoMetadata({ | ||
| title, | ||
| description, | ||
| canonicalPath, | ||
| ogImage = 'https://nest.owasp.org/img/owasp_icon_white_background.png', | ||
| noIndex = false, | ||
| keywords = [], | ||
| type = 'website', | ||
| locale = 'en_US', | ||
| siteName = 'OWASP Nest', | ||
| twitterHandle = '@owasp', | ||
| }: SeoMetadataProps): Metadata { |
There was a problem hiding this comment.
Implement the noIndex parameter to control search engine indexing.
The noIndex parameter is defined and assigned a default value but never used in the function implementation. This is also flagged in the pipeline errors.
export function generateSeoMetadata({
title,
description,
canonicalPath,
ogImage = 'https://nest.owasp.org/img/owasp_icon_white_background.png',
noIndex = false,
keywords = [],
type = 'website',
locale = 'en_US',
siteName = 'OWASP Nest',
twitterHandle = '@owasp',
}: SeoMetadataProps): Metadata {
const baseUrl = process.env.NEXT_PUBLIC_BASE_URL || 'https://nest.owasp.org'
const formattedTitle = title.includes('OWASP Nest') ? title : `${title} – OWASP Nest`
const canonicalUrl = canonicalPath ? `${baseUrl}${canonicalPath}` : undefined
return {
metadataBase: new URL(baseUrl),
title: formattedTitle,
description,
+ ...(noIndex && {
+ robots: {
+ index: false,
+ follow: true,
+ }
+ }),
...(keywords.length > 0 && { keywords: keywords.join(', ') }),📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function generateSeoMetadata({ | |
| title, | |
| description, | |
| canonicalPath, | |
| ogImage = 'https://nest.owasp.org/img/owasp_icon_white_background.png', | |
| noIndex = false, | |
| keywords = [], | |
| type = 'website', | |
| locale = 'en_US', | |
| siteName = 'OWASP Nest', | |
| twitterHandle = '@owasp', | |
| }: SeoMetadataProps): Metadata { | |
| export function generateSeoMetadata({ | |
| title, | |
| description, | |
| canonicalPath, | |
| ogImage = 'https://nest.owasp.org/img/owasp_icon_white_background.png', | |
| noIndex = false, | |
| keywords = [], | |
| type = 'website', | |
| locale = 'en_US', | |
| siteName = 'OWASP Nest', | |
| twitterHandle = '@owasp', | |
| }: SeoMetadataProps): Metadata { | |
| const baseUrl = process.env.NEXT_PUBLIC_BASE_URL || 'https://nest.owasp.org' | |
| const formattedTitle = title.includes('OWASP Nest') ? title : `${title} – OWASP Nest` | |
| const canonicalUrl = canonicalPath ? `${baseUrl}${canonicalPath}` : undefined | |
| return { | |
| metadataBase: new URL(baseUrl), | |
| title: formattedTitle, | |
| description, | |
| ...(noIndex && { | |
| robots: { | |
| index: false, | |
| follow: true, | |
| }, | |
| }), | |
| ...(keywords.length > 0 && { keywords: keywords.join(', ') }), | |
| // …other metadata fields | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Run CI/CD
[error] 22-22: ESLint: 'noIndex' is assigned a value but never used. Allowed unused args must match /^_/u (@typescript-eslint/no-unused-vars)
|
@coderabbitai re review please |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 33494 🏁 Script executed: Length of output: 1543 @abhayymishraa I've completed a re-review of your PR that adds metadata support for static and dynamic pages. Here's my assessment: Overall ArchitectureThe implementation does an excellent job of providing a comprehensive metadata system with:
Implementation Strengths
Recent ImprovementsYour latest commits have further improved the implementation by:
Testing Considerations
Optimization Notes
This PR represents a thorough and well-implemented approach to managing metadata across the application. The changes follow Next.js best practices and provide a solid foundation for SEO optimization. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/server/fetchCsrfTokenServer.ts (2)
15-16: Consider adding validation for the csrftoken.The function assumes that
data.csrftokenwill always exist and be a string, but there's no validation to ensure this.const data = await response.json() - return data.csrftoken + if (!data.csrftoken) { + throw new Error('CSRF token not found in response') + } + return data.csrftoken
17-21: Simplify error handling in catch block.The current error handling has a redundant condition. If there's no error, the catch block wouldn't execute at all.
} catch (error) { - if (error) { - throw error - } + throw error }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/server/fetchCsrfTokenServer.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/server/fetchCsrfTokenServer.ts (1)
frontend/src/utils/credentials.ts (3)
ENVIRONMENT(4-4)CSRF_URL_DOCKER(3-3)CSRF_URL(2-2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (4)
frontend/src/server/fetchCsrfTokenServer.ts (4)
1-1: Looks good - correct imports for credentials.The import statement correctly pulls in the required environment variables and constants from the credentials utility file.
3-4: URL selection logic is now correct.The conditional logic properly selects between CSRF_URL_DOCKER and CSRF_URL based on the environment. This correctly addresses the previously identified issue with inverted logic.
5-9: Good implementation of fetch request with credentials.The fetch implementation correctly:
- Uses the proper URL from environment variables
- Includes credentials which is essential for CSRF tokens
- Uses an appropriate GET method
- Is wrapped in a try/catch block for error handling
11-13: Error handling is now implemented correctly.The current implementation properly throws an error with status information when the response is not OK, addressing the previous review comment about improving error handling.
| export const fetchCsrfTokenServer = async (): Promise<string> => { | ||
| const csrfUrl = ENVIRONMENT === 'docker' ? CSRF_URL_DOCKER : CSRF_URL | ||
| try { | ||
| const response = await fetch(csrfUrl, { | ||
| credentials: 'include', | ||
| method: 'GET', | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch CSRF token: ${response.status} ${response.statusText}`) | ||
| } | ||
|
|
||
| const data = await response.json() | ||
| return data.csrftoken | ||
| } catch (error) { | ||
| if (error) { | ||
| throw error | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function lacks a return value for all code paths.
The function is typed to return Promise<string> but has a code path where it doesn't return anything (after the catch block). This could lead to undefined being returned, which doesn't match the function signature.
export const fetchCsrfTokenServer = async (): Promise<string> => {
const csrfUrl = ENVIRONMENT === 'docker' ? CSRF_URL_DOCKER : CSRF_URL
try {
const response = await fetch(csrfUrl, {
credentials: 'include',
method: 'GET',
})
if (!response.ok) {
throw new Error(`Failed to fetch CSRF token: ${response.status} ${response.statusText}`)
}
const data = await response.json()
return data.csrftoken
} catch (error) {
if (error) {
throw error
}
+ throw new Error('Failed to fetch CSRF token due to an unknown error')
}
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const fetchCsrfTokenServer = async (): Promise<string> => { | |
| const csrfUrl = ENVIRONMENT === 'docker' ? CSRF_URL_DOCKER : CSRF_URL | |
| try { | |
| const response = await fetch(csrfUrl, { | |
| credentials: 'include', | |
| method: 'GET', | |
| }) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch CSRF token: ${response.status} ${response.statusText}`) | |
| } | |
| const data = await response.json() | |
| return data.csrftoken | |
| } catch (error) { | |
| if (error) { | |
| throw error | |
| } | |
| } | |
| } | |
| export const fetchCsrfTokenServer = async (): Promise<string> => { | |
| const csrfUrl = ENVIRONMENT === 'docker' ? CSRF_URL_DOCKER : CSRF_URL | |
| try { | |
| const response = await fetch(csrfUrl, { | |
| credentials: 'include', | |
| method: 'GET', | |
| }) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch CSRF token: ${response.status} ${response.statusText}`) | |
| } | |
| const data = await response.json() | |
| return data.csrftoken | |
| } catch (error) { | |
| if (error) { | |
| throw error | |
| } | |
| throw new Error('Failed to fetch CSRF token due to an unknown error') | |
| } | |
| } |
There was a problem hiding this comment.
It's a working PoC from my point of view. I have a couple of questions though:
- I didn't get the whole point of the new
_DOCKERenv variables -- it all works after removing them (I might be missing something as I don't have much experience with Next.js/SSR) - Can we somehow test the new functionality?
Please also look into optimizing the following:
| } catch (error) { | ||
| if (error) { | ||
| throw error | ||
| } |
There was a problem hiding this comment.
Could you explain this logic?
There was a problem hiding this comment.
Actually what in order to access the csrf token on server inside docker setup we can't just send the request to localhost:3000 because inside docker it can contact other containers through container_name
| @@ -0,0 +1,36 @@ | |||
| import { Metadata } from 'next' | |||
| import React from 'react' | |||
| import { GET_ORGANIZATION_DATA } from 'server/queries/organizationQueries' | |||
There was a problem hiding this comment.
The existing queries are too heavy for layout purposes -- they return a lot of information that is not going to be used at all.
There was a problem hiding this comment.
Yes i will work on that and make separate query for the metadata
| @@ -0,0 +1,38 @@ | |||
| import { Metadata } from 'next' | |||
| import React from 'react' | |||
| import { GET_REPOSITORY_DATA } from 'server/queries/repositoryQueries' | |||
There was a problem hiding this comment.
Same here -- you only need name/description but request all this data
for the first point : for the testing purpose i will look forward whatever i can do to test this new functionality. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/server/fetchCsrfToken.ts (1)
5-5: Consider adding environment variable validation.The code assumes that
process.env.NEXT_SERVER_CSRF_URLwill always be defined. Consider adding validation or a fallback value to handle cases where this environment variable might be missing, especially during development or testing.- const response = await fetch(process.env.NEXT_SERVER_CSRF_URL, { + const csrfUrl = process.env.NEXT_SERVER_CSRF_URL + if (!csrfUrl) { + throw new AppError(500, 'NEXT_SERVER_CSRF_URL environment variable is not defined') + } + const response = await fetch(csrfUrl, {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/.env.example(1 hunks)frontend/src/app/chapters/[chapterKey]/layout.tsx(1 hunks)frontend/src/app/committees/[committeeKey]/layout.tsx(1 hunks)frontend/src/app/members/[memberKey]/layout.tsx(1 hunks)frontend/src/app/organizations/[organizationKey]/layout.tsx(1 hunks)frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/layout.tsx(1 hunks)frontend/src/app/projects/[projectKey]/layout.tsx(1 hunks)frontend/src/app/snapshots/[id]/layout.tsx(1 hunks)frontend/src/server/fetchCsrfToken.ts(1 hunks)frontend/src/server/fetchCsrfTokenServer.ts(1 hunks)frontend/src/utils/helpers/apolloClientServer.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/.env.example
- frontend/src/app/committees/[committeeKey]/layout.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/server/fetchCsrfTokenServer.ts
- frontend/src/app/organizations/[organizationKey]/layout.tsx
- frontend/src/utils/helpers/apolloClientServer.ts
- frontend/src/app/snapshots/[id]/layout.tsx
- frontend/src/app/organizations/[organizationKey]/repositories/[repositoryKey]/layout.tsx
- frontend/src/app/members/[memberKey]/layout.tsx
- frontend/src/app/projects/[projectKey]/layout.tsx
- frontend/src/app/chapters/[chapterKey]/layout.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
frontend/src/server/fetchCsrfToken.ts (1)
5-5: Using environment variable directly improves configuration management.The change to use
process.env.NEXT_SERVER_CSRF_URLdirectly instead of importing a constant aligns with centralized configuration practices and supports the new metadata infrastructure. This approach makes the application more configurable across different environments.
There was a problem hiding this comment.
Okay, after reading your comments and taking another look into this the actual problem seem to be in a bit different dimension. It look lime Next.js server side URLs need to be set differently. We don't need another environment (docker is not in the same dimension w/ local/staging/production).
I updated code to what at least works locally for me and removed all try/catch blocks (this is really bad practice, think twice before using this approach again).
Now we need to fix e2e tests, I guess it's about CSRF but I'm not really sure.
There was a problem hiding this comment.
Shouldn't this be under server/ directory?
There was a problem hiding this comment.
yes, i think both fetchcsrftoken and apolloserverclient will be under server/ directory
|
Resolves #(put the issue number here)
Add the PR description here.