fixes #2124: Refactored environment variable handling from credentials.ts to env.server.ts and env.client.ts #2125
Conversation
Summary by CodeRabbit
WalkthroughRefactors environment handling by introducing a server-only module ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labelsfrontend, frontend-tests Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/server/fetchMetricsPDF.ts (1)
1-35: Client-only code placed under server directory; enforce client-only and/or move the fileThis module uses
window,document, and triggers a download—browser-only APIs. Keeping it undersrc/serveris misleading and increases the risk of SSR import crashes.Apply this diff to enforce client-only usage immediately:
+import 'client-only' import { handleAppError } from 'app/global-error' import { API_URL } from 'utils/env.client'Follow-up (recommended):
- Move this file to a client/shared location (e.g.,
src/utils/downloads/fetchMetricsPDF.ts) to reflect its runtime.- Ensure all importers are client components/modules or lazy-load it on the client.
frontend/src/app/api/auth/[...nextauth]/route.ts (2)
81-83: Guard against undefined session.user in jwt callback update trigger.Accessing session.user without a null-check can throw. Use optional chaining or a default.
- if (trigger === 'update' && session) { - token.isOwaspStaff = (session as ExtendedSession).user.isOwaspStaff || false - } + if (trigger === 'update' && session) { + token.isOwaspStaff = Boolean((session as ExtendedSession)?.user?.isOwaspStaff) + }
8-20: Incorrect Error construction in catch blocks; use cause to preserve the original error.
new Error(message, err)is invalid. Either log the original error or use thecauseoption.- } catch (err) { - throw new Error('Failed to fetch project leader status Error', err) - } + } catch (err) { + throw new Error('Failed to fetch project leader status', { cause: err as unknown }) + }- } catch (err) { - throw new Error('Failed to fetch mentor status Error', err) - } + } catch (err) { + throw new Error('Failed to fetch mentor status', { cause: err as unknown }) + }Also applies to: 22-34
🧹 Nitpick comments (19)
frontend/src/utils/env.server.ts (2)
3-4: Avoid non-null assertions on env vars; keep them optional and validate at use sitesUsing
!masks misconfiguration at compile time and can lead to subtle runtime issues if the vars are missing. Prefer optional types and explicit validation.Apply this diff to drop the non-null assertions:
-export const GITHUB_CLIENT_ID = process.env.NEXT_SERVER_GITHUB_CLIENT_ID!; -export const GITHUB_CLIENT_SECRET = process.env.NEXT_SERVER_GITHUB_CLIENT_SECRET!; +export const GITHUB_CLIENT_ID = process.env.NEXT_SERVER_GITHUB_CLIENT_ID; +export const GITHUB_CLIENT_SECRET = process.env.NEXT_SERVER_GITHUB_CLIENT_SECRET;If you want stronger guarantees, consider a small validator (e.g., zod) that throws during startup when a var is required.
6-6: Sentry token name: support the conventional SENTRY_AUTH_TOKEN as a fallbackMost CI/CD setups export
SENTRY_AUTH_TOKEN. Restricting toNEXT_SENTRY_AUTH_TOKENrisks breaking releases if envs aren’t updated. Support both.Apply this diff:
-export const SENTRY_AUTH_TOKEN = process.env.NEXT_SENTRY_AUTH_TOKEN +export const SENTRY_AUTH_TOKEN = + process.env.SENTRY_AUTH_TOKEN ?? process.env.NEXT_SENTRY_AUTH_TOKENPlease confirm which key is set in your deployment secrets (CI, Vercel, etc.).
frontend/src/server/fetchAlgoliaData.ts (1)
14-16: Avoid mutating input parameters; derive an effective filter list insteadMutating
facetFilterscan surprise callers. Build a new array for readability and purity.Apply this diff:
- if (['projects', 'chapters'].includes(indexName)) { - facetFilters.push('idx_is_active:true') - } + const effectiveFacetFilters = + ['projects', 'chapters'].includes(indexName) + ? [...facetFilters, 'idx_is_active:true'] + : facetFilters @@ - facetFilters, + facetFilters: effectiveFacetFilters,Also applies to: 30-35
frontend/src/server/fetchCsrfToken.ts (2)
4-9: Fail fast if CSRF_URL is missingA clear early error improves debuggability;
fetch(undefined)will otherwise yield a less obvious failure.Apply this diff:
export const fetchCsrfToken = async (): Promise<string> => { try { + if (!CSRF_URL) { + throw new AppError(500, 'CSRF_URL is not defined') + } const response = await fetch(CSRF_URL, { credentials: 'include', method: 'GET', })
28-29: Type-safe error message extraction
errorisunknownincatch; optional chaining on it isn’t type-safe. Narrow before reading.message.Apply this diff:
- const message = error?.message || 'Unexpected error while fetching CSRF token' + const message = + error instanceof Error ? error.message : 'Unexpected error while fetching CSRF token'frontend/src/utils/helpers/apolloClient.ts (1)
6-16: Optional: avoid module-scope instantiation to prevent SSR/edge initialization surprisesToday apolloClient is created at import time. In mixed SSR/CSR contexts this can complicate testing and error handling when GRAPHQL_URL is misconfigured. Consider lazy-init via a getter to reduce side effects and surface errors at call time.
-const createApolloClient = () => { +const createApolloClient = () => { if (!GRAPHQL_URL) { const error = new AppError(500, 'Missing GraphQL URL') handleAppError(error) return null } … } -const apolloClient = createApolloClient() +let apolloClient: ApolloClient<any> | null = null +export const getApolloClient = () => { + if (!apolloClient) apolloClient = createApolloClient() + return apolloClient +} -export default apolloClient +export default getApolloClient()frontend/src/app/settings/api-keys/layout.tsx (1)
4-9: Correct gating via server-only env; consider centralizing the guardImporting isGithubAuthEnabled from utils/env.server and gating with notFound() is spot on. To DRY similar checks across layouts/pages, consider a tiny helper (e.g., assertGithubAuthEnabled()) that throws notFound internally.
Example helper (new file suggestion, not required):
// frontend/src/utils/guards.ts import { notFound } from 'next/navigation' import { isGithubAuthEnabled } from 'utils/env.server' export function assertGithubAuthEnabled() { if (!isGithubAuthEnabled()) notFound() }Then call assertGithubAuthEnabled() at the top of server layouts/pages that need the gate.
frontend/src/app/projects/dashboard/layout.tsx (1)
3-13: Consistent move to server-only env import; gate is correctSame pattern here looks good: server component + utils/env.server import + notFound() guard. Keeps secrets off the client boundary and matches the PR objective.
Small nit: if this check is duplicated in multiple places, consider reusing a shared guard as noted in the API Keys layout comment.
frontend/src/instrumentation-client.ts (1)
2-9: Client-safe env import for Sentry is correct; add a safe default for ENVIRONMENTSwitching ENVIRONMENT/RELEASE_VERSION/SENTRY_DSN to utils/env.client is correct. To harden against missing ENVIRONMENT at runtime in dev/test, consider a safe default when lowercasing.
- environment: ENVIRONMENT.toLowerCase(), + environment: (ENVIRONMENT || 'development').toLowerCase(),Alternatively, export a non-empty default for ENVIRONMENT from env.client.ts so call sites can remain simple.
frontend/src/app/layout.tsx (1)
6-7: Client/Server Environment Checks PassedAll of your setup around
env.server.tsandenv.client.tsis correct:
env.server.tsis markedserver-only.- No client components import from
utils/env.server.env.client.tsonly readsNEXT_PUBLIC_*variables.You can safely proceed, but here are two optional refactors to tighten things up:
- In
frontend/src/app/layout.tsx(around lines 6–7), guard the<GoogleAnalytics>component so it only renders whenGTM_IDis truthy. This prevents injecting a tag with an empty or undefined ID.- Double-check the format of
GTM_ID. If it holds a GA4 measurement ID (G-XXXX…), continue using<GoogleAnalytics>. If it’s actually a Tag Manager ID (GTM-XXXX…), switch to Next’sGoogleTagManagercomponent instead.frontend/src/sentry.server.config.ts (1)
2-2: Importing Sentry env from env.client is acceptable; add safe fallbacks.Given server config runs only on the server, reading browser-safe values here is fine if these are indeed NEXT_PUBLIC_* variables. Add defensive defaults to avoid runtime errors when envs are missing (e.g., ENVIRONMENT?.toLowerCase?.() ?? 'development').
You could harden the init call:
Sentry.init({ dsn: SENTRY_DSN || '', enabled: Boolean(SENTRY_DSN), environment: (ENVIRONMENT || 'development').toLowerCase(), release: RELEASE_VERSION || undefined, replaysOnErrorSampleRate: 0.5, replaysSessionSampleRate: 0.5, tracesSampleRate: 0.5, })Please confirm env.client actually exports SENTRY_DSN, ENVIRONMENT, and RELEASE_VERSION from NEXT_PUBLIC_* variables so none of these are secret-bearing.
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (2)
128-128: Minor type formatting nit.Add a space after the semicolon for readability in the inline type:
{ data: unknown[]; [key: string]: unknown }.
159-159: Minor type formatting nit.Same readability nit here:
{ leaders: string; [key: string]: unknown }.frontend/src/app/api/auth/[...nextauth]/route.ts (1)
6-6: Correct: server-only isGithubAuthEnabled import.Nice move to env.server, consistent with the split. For consistency, consider sourcing GitHub clientId/clientSecret from env.server as well (instead of process.env) to centralize validation and defaults.
Example:
import { isGithubAuthEnabled, GITHUB_CLIENT_ID, GITHUB_CLIENT_SECRET } from 'utils/env.server' // ... GitHubProvider({ clientId: GITHUB_CLIENT_ID, clientSecret: GITHUB_CLIENT_SECRET, // ... })frontend/src/components/Footer.tsx (1)
47-49: Remove duplicated Tailwind class in collapsible panellg:max-h-full is present both in the base and the false branch. Drop the duplicate to keep the class list minimal.
Apply this diff:
- className={`space-y-2 overflow-hidden text-sm transition-all duration-300 ease-in-out lg:max-h-full ${openSection === section.title ? 'max-h-96' : 'max-h-0 lg:max-h-full' - }`} + className={`space-y-2 overflow-hidden text-sm transition-all duration-300 ease-in-out lg:max-h-full ${ + openSection === section.title ? 'max-h-96' : 'max-h-0' + }`}frontend/src/components/CardDetailsPage.tsx (2)
166-183: Guard against possibly undefined statsIf stats can be absent for some entity types, stats.map will throw. Consider optional chaining to be safe.
Apply this diff:
- > - {stats.map((stat, index) => ( + > + {stats?.map((stat, index) => (
98-104: Add a null/array guard for healthMetricsData to prevent crashesBoth locations assume healthMetricsData is an array. If undefined/null ever reaches here for projects, .length will throw.
Apply this diff in both places:
- {IS_PROJECT_HEALTH_ENABLED && type === 'project' && healthMetricsData.length > 0 && ( + {IS_PROJECT_HEALTH_ENABLED && + type === 'project' && + Array.isArray(healthMetricsData) && + healthMetricsData.length > 0 && ( <MetricsScoreCircle- {IS_PROJECT_HEALTH_ENABLED && type === 'project' && healthMetricsData.length > 0 && ( + {IS_PROJECT_HEALTH_ENABLED && + type === 'project' && + Array.isArray(healthMetricsData) && + healthMetricsData.length > 0 && ( <HealthMetrics data={healthMetricsData} />Also applies to: 304-306
frontend/src/utils/env.client.ts (1)
1-1: Enforce client-only usage of this moduleFor symmetry with env.server.ts using server-only, consider marking this module as client-only to catch accidental server imports at build time.
Apply at the top:
+import 'client-only'frontend/__tests__/unit/pages/Login.test.tsx (1)
18-20: Place env.server mock before importing the page moduleLoginPage imports isGithubAuthEnabled at module scope. To avoid any edge cases with mock hoisting across ts-jest/babel-jest setups, move this mock above the import of app/auth/login/page (or dynamically import after mocking).
Example layout:
+jest.mock('utils/env.server', () => ({ + isGithubAuthEnabled: jest.fn(() => true), +})) -import LoginPage from 'app/auth/login/page' +const { default: LoginPage } = require('app/auth/login/page')Alternatively, keep static import but place all jest.mock(...) calls before it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(3 hunks)frontend/__tests__/unit/components/Footer.test.tsx(1 hunks)frontend/__tests__/unit/pages/Login.test.tsx(3 hunks)frontend/next.config.ts(1 hunks)frontend/src/app/api/auth/[...nextauth]/route.ts(2 hunks)frontend/src/app/auth/login/page.tsx(1 hunks)frontend/src/app/layout.tsx(1 hunks)frontend/src/app/projects/dashboard/layout.tsx(1 hunks)frontend/src/app/settings/api-keys/layout.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(4 hunks)frontend/src/components/Footer.tsx(2 hunks)frontend/src/instrumentation-client.ts(1 hunks)frontend/src/sentry.server.config.ts(1 hunks)frontend/src/server/fetchAlgoliaData.ts(1 hunks)frontend/src/server/fetchCsrfToken.ts(1 hunks)frontend/src/server/fetchMetricsPDF.ts(1 hunks)frontend/src/utils/env.client.ts(1 hunks)frontend/src/utils/env.server.ts(1 hunks)frontend/src/utils/helpers/apolloClient.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-10T11:08:47.258Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/api/auth/[...nextauth]/route.ts:13-25
Timestamp: 2025-08-10T11:08:47.258Z
Learning: In the OWASP Nest codebase (frontend/src/app/api/auth/[...nextauth]/route.ts), input validation and string trimming for authentication-related queries like `isProjectLeader` and `isMentor` are handled in the backend rather than the frontend. The backend is responsible for sanitizing and validating input parameters.
Applied to files:
frontend/src/app/api/auth/[...nextauth]/route.ts
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/app/api/auth/[...nextauth]/route.tsfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/Login.test.tsx
🧬 Code graph analysis (2)
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
frontend/src/types/auth.ts (1)
ExtendedSession(8-20)
frontend/__tests__/unit/pages/Login.test.tsx (1)
frontend/src/app/auth/login/page.tsx (1)
LoginPage(4-6)
🔇 Additional comments (14)
frontend/src/utils/env.server.ts (2)
1-1: Good call adding the server-only guardImporting
server-onlyprevents accidental client-side usage of this module. This aligns with the PR’s main objective.
8-10: LGTM: helper correctly gates GitHub auth on both keysThe boolean check is clear and side-effect free.
frontend/src/server/fetchAlgoliaData.ts (1)
3-3: LGTM: using env.client for IDX_URL keeps it clearly browser-safeImporting
IDX_URLfrom the public env module avoids any server-secret leakage while keeping usage centralized.frontend/src/server/fetchMetricsPDF.ts (1)
2-2: LGTM on env refactorSourcing
API_URLfromenv.clientis correct for a browser-only helper.frontend/src/utils/helpers/apolloClient.ts (1)
4-4: Approved: GraphQL URL client-safe import verifiedAll audit checks have passed, confirming that the GraphQL URL is now safely scoped to client code without any server-only or credential leaks:
frontend/src/utils/env.client.tsexportsGRAPHQL_URL(fromNEXT_PUBLIC_GRAPHQL_URL).frontend/src/utils/env.server.tsdoes not exportGRAPHQL_URLand is protected by aserver-onlyimport.- No client components (marked with
"use client") import fromutils/env.server.- There are no remaining imports from
utils/credentialsanywhere infrontend/.Looks good to merge!
frontend/src/app/auth/login/page.tsx (1)
1-5: Server-only import is correct for isGithubAuthEnabled in a server componentThis page is a server component (no "use client"), so importing from utils/env.server is appropriate and enforces the intended boundary. The prop name casing (isGitHubAuthEnabled) remains consistent with existing component API.
Confirm that utils/env.server.ts includes
import 'server-only'at the top and that no client-marked modules import isGithubAuthEnabled (script in my earlier comment covers this).frontend/__tests__/unit/components/Footer.test.tsx (1)
98-101: Test mocks correctly migrated to env.client.Mocking ENVIRONMENT and RELEASE_VERSION from utils/env.client keeps tests aligned with the new split. No issues.
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
72-74: Mock source correctly switched to env.client.This aligns with production code moving IS_PROJECT_HEALTH_ENABLED to client-safe envs.
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
36-55: Verify NextAuth providers configuration when GitHub auth is disabledWe ran a search for any existing
CredentialsProviderusage and found none, so if GitHub auth is turned off theprovidersarray will indeed be empty. It’s not guaranteed that NextAuth will initialize correctly without at least one provider—please manually verify that disabling GitHub auth does not trigger a runtime error.• File to test:
–frontend/src/app/api/auth/[...nextauth]/route.ts(lines 36–55)
• Verification steps:
- Disable GitHub auth and start the app.
- Hit the NextAuth route and confirm it doesn’t error.
• If it fails at runtime, consider one of:
– Returning HTTP 503 from this route when auth is disabled
– Introducing a minimalCredentialsProvidergated by the same feature flagfrontend/src/components/Footer.tsx (1)
10-10: Client-safe env import looks correctSwitching to utils/env.client aligns with the PR’s server/client split and prevents accidental bundling of secrets. No action needed.
frontend/src/components/CardDetailsPage.tsx (1)
285-289: LGTM on PRs/Releases gridThe layout and props are consistent with the rest of the component and the broader refactor.
frontend/src/utils/env.client.ts (2)
8-8: Boolean normalization reads wellThe strict equality to 'true' is explicit and avoids accidental truthiness leaks. No changes needed.
1-11: I've updated the scan to correctly pass a match-all pattern tofdand separately verify the components directory. This will ensure we catch anyenv.serverimports in client-context files. Let me know the results!frontend/__tests__/unit/pages/Login.test.tsx (1)
25-25: LGTM: explicit semicolons to avoid ASI pitfalls in TSThe leading semicolons before IIFEs/type-cast lines are fine and consistent. Session state mocks read cleanly.
Also applies to: 35-35, 41-41, 55-55, 62-62
| import { withSentryConfig } from '@sentry/nextjs' | ||
| import type { NextConfig } from 'next' | ||
| import { SENTRY_AUTH_TOKEN } from 'utils/credentials' | ||
| import { SENTRY_AUTH_TOKEN } from 'utils/env.server' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid importing app code into next.config; read env directly to prevent loader/alias pitfalls
next.config.ts runs in Node during build. Pulling from utils/env.server can: (a) rely on TS path aliases that may not be honored here, and (b) unnecessarily pull in the server-only side-effect. Reading the env directly is simpler and more robust.
Apply this diff:
-import { SENTRY_AUTH_TOKEN } from 'utils/env.server'
+const SENTRY_AUTH_TOKEN =
+ process.env.SENTRY_AUTH_TOKEN ?? process.env.NEXT_SENTRY_AUTH_TOKENTo verify alias and Next support in your repo, run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Next version:"
jq -r '.dependencies.next // .devDependencies.next // "unknown"' package.json 2>/dev/null || echo "package.json not found at repo root"
echo
echo "TS path alias for 'utils/*':"
rg -nC2 '"paths"\s*:\s*{[^}]*"utils/\*"' tsconfig*.json || trueLength of output: 493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating package.json files:"
find . -type f -name package.json
echo
echo "Inspecting Next version in each package.json:"
for pkg in $(find . -type f -name package.json); do
echo "File: $pkg"
jq -r '.dependencies.next // .devDependencies.next // "not specified"' "$pkg" || echo "failed to parse $pkg"
echo
done
echo "Locating tsconfig*.json files:"
find . -type f -name 'tsconfig*.json'
echo
echo "Inspecting compilerOptions.paths in each tsconfig*.json:"
for cfg in $(find . -type f -name 'tsconfig*.json'); do
echo "File: $cfg"
jq -r '.compilerOptions.paths // "no paths defined"' "$cfg" || echo "failed to parse $cfg"
echo
done
echo "Displaying first 20 lines of next.config.ts:"
find . -type f -name 'next.config.ts' -exec sed -n '1,20p' {} +Length of output: 2467
Avoid importing application code into next.config.ts; read environment variables directly
- Although your
frontend/tsconfig.jsondefines autils/*path alias,next.config.tsis loaded as a CommonJS module at build time and does not reliably honor TypeScript path mappings (nextjs.org). - Importing from
utils/env.serverinadvertently pulls in application-level, server-only logic (and any side-effects) into your build configuration. - A more robust pattern is to read the token directly from
process.env, with a fallback:
-import { SENTRY_AUTH_TOKEN } from 'utils/env.server'
+const SENTRY_AUTH_TOKEN =
+ process.env.SENTRY_AUTH_TOKEN ?? process.env.NEXT_SENTRY_AUTH_TOKEN📝 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.
| import { SENTRY_AUTH_TOKEN } from 'utils/env.server' | |
| const SENTRY_AUTH_TOKEN = | |
| process.env.SENTRY_AUTH_TOKEN ?? process.env.NEXT_SENTRY_AUTH_TOKEN |
🤖 Prompt for AI Agents
In frontend/next.config.ts around line 3, importing SENTRY_AUTH_TOKEN from
utils/env.server pulls application/server code into the build config and may
break due to TypeScript path aliasing; remove that import and instead read the
token directly from process.env (with a safe fallback) inside next.config.ts so
the config relies only on environment variables at build time and does not
execute app-level code.
| ; (session as ExtendedSession).accessToken = token.accessToken as string | ||
|
|
||
| if (session.user) { | ||
| ;(session as ExtendedSession).user.login = token.login as string | ||
| ;(session as ExtendedSession).user.isMentor = token.isMentor as boolean | ||
| ;(session as ExtendedSession).user.isLeader = token.isLeader as boolean | ||
| ;(session as ExtendedSession).user.isOwaspStaff = token.isOwaspStaff as boolean | ||
| ; (session as ExtendedSession).user.login = token.login as string | ||
| ; (session as ExtendedSession).user.isMentor = token.isMentor as boolean | ||
| ; (session as ExtendedSession).user.isLeader = token.isLeader as boolean | ||
| ; (session as ExtendedSession).user.isOwaspStaff = token.isOwaspStaff as boolean |
There was a problem hiding this comment.
Stray semicolons and indentation will trip linters and reduce readability.
These leading semicolons are unnecessary and inconsistent. Also coerce booleans explicitly.
Apply this diff:
- ; (session as ExtendedSession).accessToken = token.accessToken as string
+ (session as ExtendedSession).accessToken = token.accessToken as string
if (session.user) {
- ; (session as ExtendedSession).user.login = token.login as string
- ; (session as ExtendedSession).user.isMentor = token.isMentor as boolean
- ; (session as ExtendedSession).user.isLeader = token.isLeader as boolean
- ; (session as ExtendedSession).user.isOwaspStaff = token.isOwaspStaff as boolean
+ (session as ExtendedSession).user.login = token.login as string
+ (session as ExtendedSession).user.isMentor = Boolean(token.isMentor)
+ (session as ExtendedSession).user.isLeader = Boolean(token.isLeader)
+ (session as ExtendedSession).user.isOwaspStaff = Boolean(token.isOwaspStaff)
}📝 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.
| ; (session as ExtendedSession).accessToken = token.accessToken as string | |
| if (session.user) { | |
| ;(session as ExtendedSession).user.login = token.login as string | |
| ;(session as ExtendedSession).user.isMentor = token.isMentor as boolean | |
| ;(session as ExtendedSession).user.isLeader = token.isLeader as boolean | |
| ;(session as ExtendedSession).user.isOwaspStaff = token.isOwaspStaff as boolean | |
| ; (session as ExtendedSession).user.login = token.login as string | |
| ; (session as ExtendedSession).user.isMentor = token.isMentor as boolean | |
| ; (session as ExtendedSession).user.isLeader = token.isLeader as boolean | |
| ; (session as ExtendedSession).user.isOwaspStaff = token.isOwaspStaff as boolean | |
| (session as ExtendedSession).accessToken = token.accessToken as string | |
| if (session.user) { | |
| (session as ExtendedSession).user.login = token.login as string | |
| (session as ExtendedSession).user.isMentor = Boolean(token.isMentor) | |
| (session as ExtendedSession).user.isLeader = Boolean(token.isLeader) | |
| (session as ExtendedSession).user.isOwaspStaff = Boolean(token.isOwaspStaff) | |
| } |
🤖 Prompt for AI Agents
In frontend/src/app/api/auth/[...nextauth]/route.ts around lines 88 to 94,
remove the stray leading semicolons and normalize indentation on the session
assignment lines; replace the current boolean casts with explicit coercion
(e.g., Boolean(token.isMentor)) while keeping the string assertion for login and
accessToken as needed, so the four assignments become consistently indented
without leading semicolons and the boolean fields are set using explicit
Boolean(...) conversions.
| import type { ExtendedSession } from 'types/auth' | ||
| import type { DetailsCardProps } from 'types/card' | ||
| import { IS_PROJECT_HEALTH_ENABLED } from 'utils/credentials' | ||
| import { IS_PROJECT_HEALTH_ENABLED } from 'utils/env.client' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Importing IS_PROJECT_HEALTH_ENABLED from env.client is correct, but ensure this file is a Client Component
This component uses useSession, useRouter, and window.location, which require a Client Component boundary. If this file isn’t already under a client parent, add the directive.
Apply at the very top of this file:
+ 'use client'If you intentionally rely on a client parent wrapper, please confirm that no server component imports this directly.
🏁 Script executed:
#!/usr/bin/env bash
set -e
FILE="frontend/src/components/CardDetailsPage.tsx"
echo "1) Checking for 'use client' directive in $FILE"
grep -n "^[\"']use client[\"']" "$FILE" || echo "=> No 'use client' directive found"
echo
echo "2) Checking for client-specific hooks and browser APIs in $FILE"
grep -E "useSession|useRouter|window\." -n "$FILE" || echo "=> No client-specific usage found"
echo
echo "3) Searching for imports of CardDetailsPage across TSX files"
rg "CardDetailsPage" -n --glob "*.tsx" .Length of output: 12861
Add the 'use client' directive to CardDetailsPage.tsx
CardDetailsPage.tsx imports and uses next-auth’s useSession, next/navigation’s useRouter, and directly references window.location, so it must be a Client Component. There is currently no 'use client' directive at the top of the file.
Apply this change at the very top of frontend/src/components/CardDetailsPage.tsx:
+ 'use client'This ensures Next.js treats the component as client-only and enables proper runtime for these hooks and browser APIs.
📝 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.
| import { IS_PROJECT_HEALTH_ENABLED } from 'utils/env.client' | |
| 'use client' | |
| import { IS_PROJECT_HEALTH_ENABLED } from 'utils/env.client' | |
| // … rest of CardDetailsPage.tsx … |
🤖 Prompt for AI Agents
In frontend/src/components/CardDetailsPage.tsx around line 17, the file uses
next-auth's useSession, next/navigation's useRouter, and window.location but
lacks the 'use client' directive; add a single line 'use client' at the very top
of the file (above all imports) so Next.js treats this component as a Client
Component and enables client-side hooks and browser APIs.
| <button | ||
| type="button" | ||
| className="flex items-center justify-center gap-2 text-nowrap rounded-md border border-[#0D6EFD] bg-transparent px-2 py-2 text-[#0D6EFD] text-blue-600 transition-all hover:bg-[#0D6EFD] hover:text-white dark:border-sky-600 dark:text-sky-600 dark:hover:bg-sky-100" | ||
| className="flex items-center justify-center gap-2 text-nowrap rounded-md border border-[#0D6EFD] bg-transparent px-2 py-2 text-[#0D6EFD] transition-all hover:bg-[#0D6EFD] hover:text-white dark:border-sky-600 dark:text-sky-600 dark:hover:bg-sky-100" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix incorrect Tailwind class and prefer theme tokens over hard-coded hex
- text-nowrap isn’t a Tailwind class; use whitespace-nowrap.
- Prefer Tailwind color tokens for consistency with themes and future palette changes.
Apply this diff:
- className="flex items-center justify-center gap-2 text-nowrap rounded-md border border-[#0D6EFD] bg-transparent px-2 py-2 text-[#0D6EFD] transition-all hover:bg-[#0D6EFD] hover:text-white dark:border-sky-600 dark:text-sky-600 dark:hover:bg-sky-100"
+ className="flex items-center justify-center gap-2 whitespace-nowrap rounded-md border bg-transparent px-2 py-2 text-blue-600 transition-all hover:bg-blue-600 hover:text-white dark:border-sky-600 dark:text-sky-600 dark:hover:bg-sky-600/10"If the specific brand shade (#0D6EFD) is required, consider adding it to the Tailwind theme as a custom color token instead of inlining the hex.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/CardDetailsPage.tsx around line 90, the element uses
a non-existent Tailwind class "text-nowrap" and hard-coded hex colors; replace
"text-nowrap" with Tailwind's "whitespace-nowrap" and swap hard-coded hex color
classes (border-[#0D6EFD], text-[#0D6EFD], hover:bg-[#0D6EFD],
dark:border-sky-600, dark:text-sky-600, dark:hover:bg-sky-100) with appropriate
Tailwind color tokens from the theme (or add a custom token for the brand shade
in tailwind.config and use that token like border-brand, text-brand,
hover:bg-brand) to ensure theme consistency and remove inline hex usage.
|
@arkid15r Coderabbit flagged some code readability issues after I ran |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
- This is an issue that wasn't assigned to you to work on.
- You must run
make check-testlocally - This is a cosplay code.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/utils/env.server.ts (1)
1-4: Remove non-null assertions on GitHub env constants
We searched the entirefrontenddirectory and confirmed that neitherGITHUB_CLIENT_IDnorGITHUB_CLIENT_SECRETis ever used in a way that assumes they’re non-null strings (no string-method calls or imports outsideenv.server.ts). They’re only used to computeIS_GITHUB_AUTH_ENABLED, so dropping the!is safe and accurately types them asstring | undefined. You can also add a runtime guard to warn if only one value is set.– File:
frontend/src/utils/env.server.ts(lines 1–2)Suggested diff:
-export const GITHUB_CLIENT_ID = process.env.NEXT_SERVER_GITHUB_CLIENT_ID! -export const GITHUB_CLIENT_SECRET = process.env.NEXT_SERVER_GITHUB_CLIENT_SECRET! +export const GITHUB_CLIENT_ID = process.env.NEXT_SERVER_GITHUB_CLIENT_ID +export const GITHUB_CLIENT_SECRET = process.env.NEXT_SERVER_GITHUB_CLIENT_SECRETOptional guard for partial configuration:
+if ((GITHUB_CLIENT_ID && !GITHUB_CLIENT_SECRET) || (!GITHUB_CLIENT_ID && GITHUB_CLIENT_SECRET)) { + // eslint-disable-next-line no-console + console.warn( + '[env.server] Partial GitHub auth configuration detected: ' + + 'set both NEXT_SERVER_GITHUB_CLIENT_ID and NEXT_SERVER_GITHUB_CLIENT_SECRET or neither.' + ) +}frontend/src/app/settings/api-keys/layout.tsx (1)
10-15: Session redirect flow is correct; consider adding a focused test.The redirect-on-missing-session path is intact. For completeness, add a test that mocks
IS_GITHUB_AUTH_ENABLED = falseand asserts a 404 for this layout, mirroring the Login page test pattern.If helpful, I can draft a unit/integration test skeleton for this layout using your existing Next/Jest setup.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/__tests__/unit/pages/Login.test.tsx(1 hunks)frontend/src/app/api/auth/[...nextauth]/route.ts(2 hunks)frontend/src/app/auth/login/page.tsx(1 hunks)frontend/src/app/layout.tsx(2 hunks)frontend/src/app/projects/dashboard/layout.tsx(1 hunks)frontend/src/app/settings/api-keys/layout.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/Footer.tsx(1 hunks)frontend/src/utils/env.client.ts(0 hunks)frontend/src/utils/env.server.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/utils/env.client.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/app/projects/dashboard/layout.tsx
- frontend/src/components/CardDetailsPage.tsx
- frontend/tests/unit/components/CardDetailsPage.test.tsx
- frontend/src/app/layout.tsx
- frontend/src/components/Footer.tsx
- frontend/tests/unit/pages/Login.test.tsx
- frontend/src/app/auth/login/page.tsx
- frontend/src/app/api/auth/[...nextauth]/route.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/app/settings/api-keys/layout.tsx (1)
frontend/src/utils/env.server.ts (1)
IS_GITHUB_AUTH_ENABLED(6-6)
🔇 Additional comments (2)
frontend/src/utils/env.server.ts (1)
4-4: No changes needed for Sentry token configuration
I’ve confirmed that you’re importingNEXT_SENTRY_AUTH_TOKENinenv.server.tsasSENTRY_AUTH_TOKENand then explicitly passing it intowithSentryConfigin yournext.config.ts. Your.env.examplealready definesNEXT_SENTRY_AUTH_TOKEN, so source-map uploads will receive the correct token.– frontend/src/utils/env.server.ts line 4:
export const SENTRY_AUTH_TOKEN = process.env.NEXT_SENTRY_AUTH_TOKEN
– frontend/next.config.ts lines 40–43:export default withSentryConfig(nextConfig, { authToken: SENTRY_AUTH_TOKEN, disableLogger: false, org: 'owasp-org', project: 'nest-frontend', })Since the token is explicitly wired into the Sentry plugin, no further renaming or configuration changes are required.
frontend/src/app/settings/api-keys/layout.tsx (1)
4-9: LGTM: server-only gate is correctly enforced at the layout boundary.Importing
IS_GITHUB_AUTH_ENABLEDfromutils/env.serverand short-circuiting withnotFound()prevents exposure of this route when GitHub auth is disabled. This aligns with the PR’s separation goals.
…ntials.ts to env.server.ts and env.client.ts (OWASP#2125) * splitted env vars into env.client and env.server and updated the imports * removed credentials.ts * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
2 participants
Proposed change
Resolves #2124
separated the
utils/credentials.tsintoutils/env.client.ts(for browser safe environment vars) andutils/env.server.ts(for server side environment variables) and updated the importsChecklist
make check-testlocally; all checks and tests passed.