feat(i18n): implement internationalization support #1
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @ohprettyhak, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application by integrating comprehensive internationalization capabilities. The changes involve a fundamental restructuring of the project's routing and content management to support multiple languages, alongside the introduction of a user-friendly language switcher and robust middleware for locale detection and URL handling. This foundational work paves the way for a more globally accessible and localized user experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements internationalization support, a significant feature. The changes introduce a [lang] dynamic route, dictionary files for translations, and middleware for locale detection and routing. My review focuses on improving code correctness, reducing duplication, and enhancing maintainability. The most critical feedback relates to correcting the usage of params in async Server Components, which are passed as resolved objects, not Promises. Other suggestions aim to refactor duplicated logic into reusable hooks and utilities for better code quality. Overall, this is a strong implementation, and the suggested changes will make the codebase more robust and easier to maintain.
| type PageProps = { | ||
| params: Promise<{ slug: string }>; | ||
| params: Promise<{ slug: string; lang: Locale }>; | ||
| }; | ||
|
|
||
| const Page = async ({ params }: PageProps) => { | ||
| const { slug } = await params; | ||
| const { slug, lang } = await params; |
There was a problem hiding this comment.
The params prop in Next.js async Server Components is a resolved object, not a Promise. Using await params is incorrect and misleading. The type should be params: { slug: string; lang: Locale } and the await should be removed. This is a recurring pattern in this pull request that needs to be corrected in all affected page components.
| type PageProps = { | |
| params: Promise<{ slug: string }>; | |
| params: Promise<{ slug: string; lang: Locale }>; | |
| }; | |
| const Page = async ({ params }: PageProps) => { | |
| const { slug } = await params; | |
| const { slug, lang } = await params; | |
| type PageProps = { | |
| params: { slug: string; lang: Locale }; | |
| }; | |
| const Page = async ({ params }: PageProps) => { | |
| const { slug, lang } = params; |
| const Page = async ({ params }: { params: Promise<{ lang: Locale }> }) => { | ||
| const { lang } = await params; |
There was a problem hiding this comment.
The params prop in Next.js async Server Components is a resolved object, not a Promise. Using await params is incorrect. The type should be { params: { lang: Locale } } and the await should be removed from const { lang } = await params;.
| const Page = async ({ params }: { params: Promise<{ lang: Locale }> }) => { | |
| const { lang } = await params; | |
| const Page = async ({ params }: { params: { lang: Locale } }) => { | |
| const { lang } = params; |
| type PageProps = { | ||
| params: Promise<{ slug: string }>; | ||
| params: Promise<{ slug: string; lang: Locale }>; | ||
| }; | ||
|
|
||
| const Page = async ({ params }: PageProps) => { | ||
| const { slug } = await params; | ||
| const { slug, lang } = await params; |
There was a problem hiding this comment.
The params prop in Next.js async Server Components is a resolved object, not a Promise. Using await params is incorrect and misleading. The type should be params: { slug: string; lang: Locale } and the await should be removed.
| type PageProps = { | |
| params: Promise<{ slug: string }>; | |
| params: Promise<{ slug: string; lang: Locale }>; | |
| }; | |
| const Page = async ({ params }: PageProps) => { | |
| const { slug } = await params; | |
| const { slug, lang } = await params; | |
| type PageProps = { | |
| params: { slug: string; lang: Locale }; | |
| }; | |
| const Page = async ({ params }: PageProps) => { | |
| const { slug, lang } = params; |
| const Page = async ({ params }: { params: Promise<{ lang: Locale }> }) => { | ||
| const { lang } = await params; |
There was a problem hiding this comment.
The params prop in Next.js async Server Components is a resolved object, not a Promise. Using await params is incorrect. The type should be { params: { lang: Locale } } and the await should be removed from const { lang } = await params;.
| const Page = async ({ params }: { params: Promise<{ lang: Locale }> }) => { | |
| const { lang } = await params; | |
| const Page = async ({ params }: { params: { lang: Locale } }) => { | |
| const { lang } = params; |
| type RootLayoutProps = PropsWithChildren<{ | ||
| params: Promise<{ lang: string }>; | ||
| }>; | ||
|
|
||
| import { getDictionary } from "./dictionaries"; | ||
|
|
||
| const RootLayout = async ({ children, params }: RootLayoutProps) => { | ||
| const { lang } = await params; | ||
| const dict = await getDictionary(lang as Locale); |
There was a problem hiding this comment.
The params prop in Next.js async Server Components is a resolved object, not a Promise. Using await params is incorrect and misleading. The type should be adjusted, and the await should be removed. To get a typed lang parameter, you can export generateStaticParams from this layout file, which will also allow you to remove the as Locale cast.
| type RootLayoutProps = PropsWithChildren<{ | |
| params: Promise<{ lang: string }>; | |
| }>; | |
| import { getDictionary } from "./dictionaries"; | |
| const RootLayout = async ({ children, params }: RootLayoutProps) => { | |
| const { lang } = await params; | |
| const dict = await getDictionary(lang as Locale); | |
| type RootLayoutProps = PropsWithChildren<{ | |
| params: { lang: string }; | |
| }>; | |
| import { getDictionary } from "./dictionaries"; | |
| const RootLayout = async ({ children, params }: RootLayoutProps) => { | |
| const { lang } = params; | |
| const dict = await getDictionary(lang as Locale); |
| for (const locale of i18n.locales) { | ||
| const suffix = `.${locale}.mdx`; | ||
| const localeArticles = articles.filter((name) => name.endsWith(suffix)); | ||
|
|
||
| for (const name of localeArticles) { | ||
| params.push({ | ||
| lang: locale, | ||
| slug: name.replace(suffix, ""), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation with nested loops is correct, but it can be written more concisely and slightly more performantly using flatMap. This would improve readability and maintainability by reducing nesting.
return i18n.locales.flatMap((locale) => {
const suffix = `.${locale}.mdx`;
return articles
.filter((name) => name.endsWith(suffix))
.map((name) => ({
lang: locale,
slug: name.replace(suffix, ""),
}));
});|
|
||
| --- | ||
|
|
||
| ## 3. Lists |
There was a problem hiding this comment.
The heading numbering is incorrect. This section should be ## 4. Lists, and subsequent headings should be renumbered accordingly to maintain the correct document structure.
## 4. Lists
| href={ | ||
| lang === i18n.defaultLocale | ||
| ? routes.crafts | ||
| : `/${lang}${routes.crafts}` | ||
| } | ||
| > | ||
| Craft | ||
| {dict.home.sections.crafts} | ||
| </Link> | ||
| </div> | ||
|
|
||
| <ItemListSection | ||
| getItemLink={(slug) => routes.thought(slug)} | ||
| getItemLink={(slug) => | ||
| lang === i18n.defaultLocale | ||
| ? routes.thought(slug) | ||
| : `/${lang}${routes.thought(slug)}` | ||
| } | ||
| items={thoughts} | ||
| sectionHref={routes.thoughts} | ||
| title="Thoughts" | ||
| sectionHref={ | ||
| lang === i18n.defaultLocale | ||
| ? routes.thoughts | ||
| : `/${lang}${routes.thoughts}` | ||
| } | ||
| title={dict.home.sections.thoughts} | ||
| /> | ||
|
|
||
| <ItemListSection | ||
| getItemLink={(slug) => routes.article(slug)} | ||
| getItemLink={(slug) => | ||
| lang === i18n.defaultLocale | ||
| ? routes.article(slug) | ||
| : `/${lang}${routes.article(slug)}` | ||
| } | ||
| items={articles} | ||
| sectionHref={routes.articles} | ||
| title="Articles" | ||
| sectionHref={ | ||
| lang === i18n.defaultLocale | ||
| ? routes.articles | ||
| : `/${lang}${routes.articles}` | ||
| } | ||
| title={dict.home.sections.articles} |
There was a problem hiding this comment.
The logic for creating a localized URL path is repeated multiple times in this component. To improve maintainability and reduce code duplication (DRY principle), consider extracting this logic into a shared utility function.
For example, you could create a helper function in a new utils/i18n.ts file:
// utils/i18n.ts
import { i18n, type Locale } from '~/i18n-config';
export function createLocalizedPath(path: string, locale: Locale) {
if (locale === i18n.defaultLocale) {
return path;
}
// Avoid double slashes for the root path
if (path === '/') {
return `/${locale}`;
}
return `/${locale}${path}`;
}Then you can use it like this: href={createLocalizedPath(routes.crafts, lang)}. This would make the code much cleaner.
| const segments = pathname?.split("/") || []; | ||
| const firstSegment = segments[1]; | ||
| const isLocale = i18n.locales.includes(firstSegment as Locale); | ||
| const currentLocale = isLocale | ||
| ? (firstSegment as Locale) | ||
| : i18n.defaultLocale; |
There was a problem hiding this comment.
The logic to determine the current locale from the pathname is duplicated in both the LanguageSwitcher and Header components (lines 70-75). This should be extracted into a custom hook (e.g., useCurrentLocale) to improve code reuse and maintainability.
You could create a new hook hooks/use-current-locale.ts:
// hooks/use-current-locale.ts
import { usePathname } from "next/navigation";
import { i18n, type Locale } from "~/i18n-config";
export function useCurrentLocale(): Locale {
const pathname = usePathname();
const segments = pathname?.split("/") || [];
const firstSegment = segments[1];
const isLocale = i18n.locales.includes(firstSegment as Locale);
return isLocale ? (firstSegment as Locale) : i18n.defaultLocale;
}Then, both components can simply call const currentLocale = useCurrentLocale();.
| if (locale === i18n.defaultLocale) { | ||
| return NextResponse.rewrite( | ||
| new URL( | ||
| `/${locale}${pathname.startsWith("/") ? "" : "/"}${pathname}`, | ||
| request.url | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| return NextResponse.redirect( | ||
| new URL( | ||
| `/${locale}${pathname.startsWith("/") ? "" : "/"}${pathname}`, | ||
| request.url | ||
| ) | ||
| ); |
There was a problem hiding this comment.
The URL construction for redirects and rewrites can be made more robust and consistent, especially for the root path (/). The current logic /${locale}${pathname} results in a trailing slash (e.g., /ko/) for the root path, which is inconsistent with how other paths are handled in the LanguageSwitcher component. Using pathname === "/" ? "" : pathname will produce /ko for the root, which is more consistent.
| if (locale === i18n.defaultLocale) { | |
| return NextResponse.rewrite( | |
| new URL( | |
| `/${locale}${pathname.startsWith("/") ? "" : "/"}${pathname}`, | |
| request.url | |
| ) | |
| ); | |
| } | |
| return NextResponse.redirect( | |
| new URL( | |
| `/${locale}${pathname.startsWith("/") ? "" : "/"}${pathname}`, | |
| request.url | |
| ) | |
| ); | |
| if (locale === i18n.defaultLocale) { | |
| return NextResponse.rewrite( | |
| new URL( | |
| `/${locale}${pathname === "/" ? "" : pathname}`, | |
| request.url | |
| ) | |
| ); | |
| } | |
| return NextResponse.redirect( | |
| new URL( | |
| `/${locale}${pathname === "/" ? "" : pathname}`, | |
| request.url | |
| ) | |
| ); |
No description provided.