-
-
Notifications
You must be signed in to change notification settings - Fork 271
fixes #2124: Refactored environment variable handling from credentials.ts to env.server.ts and env.client.ts #2125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { isGithubAuthEnabled } from 'utils/credentials' | ||
| import { IS_GITHUB_AUTH_ENABLED } from 'utils/env.server' | ||
| import LoginPageContent from 'components/LoginPageContent' | ||
|
|
||
| export default function LoginPage() { | ||
| return <LoginPageContent isGitHubAuthEnabled={isGithubAuthEnabled()} /> | ||
| return <LoginPageContent isGitHubAuthEnabled={IS_GITHUB_AUTH_ENABLED} /> | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,7 +14,7 @@ import { useRouter } from 'next/navigation' | |||||||||||||
| import { useSession } from 'next-auth/react' | ||||||||||||||
| 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' | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainImporting 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 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 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| import { scrollToAnchor } from 'utils/scrollToAnchor' | ||||||||||||||
| import { getSocialIcon } from 'utils/urlIconMappings' | ||||||||||||||
| import AnchorTitle from 'components/AnchorTitle' | ||||||||||||||
|
|
@@ -87,7 +87,7 @@ const DetailsCard = ({ | |||||||||||||
| ) && ( | ||||||||||||||
| <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" | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Fix incorrect Tailwind class and prefer theme tokens over hard-coded hex
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.
🤖 Prompt for AI Agents |
||||||||||||||
| onClick={() => { | ||||||||||||||
| router.push(`${window.location.pathname}/edit`) | ||||||||||||||
| }} | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| export const GITHUB_CLIENT_ID = process.env.NEXT_SERVER_GITHUB_CLIENT_ID! | ||
arkid15r marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| export const GITHUB_CLIENT_SECRET = process.env.NEXT_SERVER_GITHUB_CLIENT_SECRET! | ||
| export const NEXTAUTH_URL = process.env.NEXTAUTH_URL | ||
| export const SENTRY_AUTH_TOKEN = process.env.NEXT_SENTRY_AUTH_TOKEN | ||
|
|
||
| export const IS_GITHUB_AUTH_ENABLED = Boolean(GITHUB_CLIENT_ID && GITHUB_CLIENT_SECRET) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Avoid importing app code into next.config; read env directly to prevent loader/alias pitfalls
next.config.tsruns in Node during build. Pulling fromutils/env.servercan: (a) rely on TS path aliases that may not be honored here, and (b) unnecessarily pull in theserver-onlyside-effect. Reading the env directly is simpler and more robust.Apply this diff:
To verify alias and Next support in your repo, run:
🏁 Script executed:
Length of output: 493
🏁 Script executed:
Length of output: 2467
Avoid importing application code into
next.config.ts; read environment variables directlyfrontend/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).utils/env.serverinadvertently pulls in application-level, server-only logic (and any side-effects) into your build configuration.process.env, with a fallback:📝 Committable suggestion
🤖 Prompt for AI Agents