i18n support for sicp #3133
i18n support for sicp #3133
Conversation
now lang in url only changes the lang in local storage and redirects to the original page
…r fetching the toc.json
There was a problem hiding this comment.
Pull Request Overview
This PR introduces internationalization support for the SICP pages by reading language parameters from the URL and local storage, fetching language-specific content, and adding a toggle UI to switch languages.
- Added a language segment to the SICP route configuration
- Refactored the table of contents component to fetch TOC JSON per language with loading and fallback
- Enhanced the main SICP page to initialize, persist, and toggle language, and to redirect based on the language parameter
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/routes/routerConfig.tsx | Added :param_lang to the SICP route and updated route definitions |
| src/pages/sicp/subcomponents/SicpToc.tsx | Refactored TOC into a loading/fallback pattern with language fetch |
| src/pages/sicp/Sicp.tsx | Handled URL/local-storage language, added toggle button, and redirects |
| src/features/sicp/utils/SicpUtils.ts | Introduced language storage helpers and default-language constants |
|
Please take whatever Copilot says with a big mountain of salt, I will review it properly later… |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes introduce bilingual support (English and Chinese) for the SICP textbook interface. Language preference is now managed via local storage, URL parameters, and a UI toggle. The table of contents and content fetching logic are updated to dynamically load language-specific resources. Routing is enhanced to recognize language-prefixed URLs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SicpPage
participant LocalStorage
participant Router
participant TOCComponent
participant Backend
User->>SicpPage: Visit /sicpjs or /sicpjs/:param_lang/:section?
SicpPage->>LocalStorage: Read language preference
SicpPage->>Router: Parse URL for :param_lang
alt Language in URL
SicpPage->>LocalStorage: Set language from URL
SicpPage->>Router: Navigate to URL without :param_lang
end
SicpPage->>TOCComponent: Render with selected language
TOCComponent->>Backend: Fetch TOC JSON for language
alt Fetch fails
TOCComponent->>TOCComponent: Load static fallback TOC
end
User->>SicpPage: Click language toggle button
SicpPage->>LocalStorage: Update language
SicpPage->>TOCComponent: Trigger re-render with new language
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Pull Request Test Coverage Report for Build 16858394883Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/routes/routerConfig.tsx (1)
52-54: Align param name and route specificity; place the 2‑segment route first and use camelCase:paramLang.Current route uses
:param_langwhileSicp.tsxreadsparamLang, causing the param to be undefined. Also, put the language-prefixed route before the generic one to avoid ambiguous matches.Apply this diff:
{ path: 'contributors', lazy: Contributors }, { path: 'callback/github', lazy: GitHubCallback }, - { path: 'sicpjs/:section?', lazy: Sicp }, - { path: 'sicpjs/:param_lang/:section?', lazy: Sicp }, + { path: 'sicpjs/:paramLang/:section?', lazy: Sicp }, + { path: 'sicpjs/:section?', lazy: Sicp }, { path: 'features', lazy: Features }src/pages/sicp/subcomponents/SicpToc.tsx (1)
24-26: FlattenTocprops instead of passing a nestedpropsobject.This avoids an extra layer and improves readability.
-const Toc: React.FC<{ toc: TreeNodeInfo[]; props: TocProps }> = ({ toc, props }) => { +const Toc: React.FC<TocProps & { toc: TreeNodeInfo[] }> = ({ toc, handleCloseToc }) => { ... - const handleNodeClicked = React.useCallback( + const handleNodeClicked = React.useCallback( (node: TreeNodeInfo) => { - if (props.handleCloseToc) { - props.handleCloseToc(); - } + if (handleCloseToc) handleCloseToc(); navigate('/sicpjs/' + String(node.nodeData)); }, - [navigate, props] + [navigate, handleCloseToc] ); ... - <Toc toc={fallbackToc as TreeNodeInfo[]} props={props} /> + <Toc toc={fallbackToc as TreeNodeInfo[]} {...props} /> ) : ( - <Toc toc={toc} props={props} /> + <Toc toc={toc} {...props} /> )}Also applies to: 101-104
src/pages/sicp/Sicp.tsx (1)
57-59: Param mismatch with router; destructure snake_case or rename the route param.If you keep the route as
:param_lang, destructure it as below to avoidundefined. If you adopt the router rename to:paramLang(recommended), this change isn’t needed.- const { paramLang, section } = useParams<{ paramLang: string; section: string }>(); + const { param_lang: paramLang, section } = + useParams<{ param_lang?: string; section?: string }>();
🧹 Nitpick comments (8)
src/features/sicp/utils/SicpUtils.ts (2)
18-21: Prefer a named constant for the event and dispatch aCustomEventwith the language payload.Improves resilience (one source of truth for the event name) and lets listeners react without re-reading LS.
+export const SICP_LANG_CHANGE_EVENT = 'sicp-tb-lang-change' as const; + export const setSicpLangLocalStorage = (value: string) => { setLocalStorage(SICP_TB_LANG_KEY, value); - window.dispatchEvent(new Event('sicp-tb-lang-change')); + window.dispatchEvent(new CustomEvent(SICP_LANG_CHANGE_EVENT, { detail: { lang: value } })); };
23-26: Optionally expose a narrower type for the language value.If you standardize the available languages, returning a union helps avoid accidental values across the app.
-export const readSicpLangLocalStorage = () => { - const data = readLocalStorage(SICP_TB_LANG_KEY, SICP_DEF_TB_LANG); - return data; -}; +export type SicpLang = 'en' | 'zh_CN'; +export const readSicpLangLocalStorage = (): SicpLang => { + return readLocalStorage(SICP_TB_LANG_KEY, SICP_DEF_TB_LANG) as SicpLang; +};src/pages/sicp/subcomponents/SicpToc.tsx (1)
88-89: Useconsole.errorfor error logging.Minor logging nit for error visibility.
- console.log(error); + console.error(error);src/pages/sicp/Sicp.tsx (5)
41-41: Type the available languages as a literal union usingas const.This improves type safety across the module and pairs well with a
SicpLangtype alias.-const AVAILABLE_SICP_TB_LANGS: readonly string[] = ['en', 'zh_CN']; +const AVAILABLE_SICP_TB_LANGS = ['en', 'zh_CN'] as const; +type SicpLang = (typeof AVAILABLE_SICP_TB_LANGS)[number];
173-174: Use the dedicated helper for section storage for consistency.Use
setSicpSectionLocalStorageinstead of rawsetLocalStorage.- setLocalStorage(SICP_CACHE_KEY, SICP_INDEX); // Prevents caching invalid page + setSicpSectionLocalStorage(SICP_INDEX); // Prevents caching invalid page
214-227: Nit: RaisezIndexof the sticky language toggle to avoid overlap issues.At
zIndex: 0, the button can end up beneath content in some stacking contexts.- <div - style={{ - position: 'sticky', - top: '20px', - left: '20px', - zIndex: 0 - }} - > + <div + style={{ + position: 'sticky', + top: '20px', + left: '20px', + zIndex: 10 + }} + >
139-147: Guarding invalidlanginside the fetch effect is fine, but consider constraining the state type.With a
SicpLangunion, this guard becomes unnecessary.
16-23: Ensure docs and tests cover the breaking i18n changes.This is a breaking change per PR summary; add:
- Routing examples with/without the language segment
- E2E/interaction tests for language toggling and persistence
- Notes on required backend (
sicpBackendUrl/json/{lang}/...) for local devI can draft a short doc section and a couple of tests if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/pages/sicp/subcomponents/__tests__/__snapshots__/SicpIndexPage.test.tsx.snapis excluded by!**/*.snapsrc/pages/sicp/subcomponents/__tests__/__snapshots__/SicpToc.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/features/sicp/utils/SicpUtils.ts(1 hunks)src/pages/sicp/Sicp.tsx(7 hunks)src/pages/sicp/subcomponents/SicpToc.tsx(3 hunks)src/routes/routerConfig.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/pages/sicp/subcomponents/SicpToc.tsx (1)
src/features/sicp/utils/SicpUtils.ts (1)
readSicpLangLocalStorage(23-26)
src/features/sicp/utils/SicpUtils.ts (1)
src/commons/utils/LocalStorageHelper.ts (2)
setLocalStorage(6-8)readLocalStorage(1-4)
src/pages/sicp/Sicp.tsx (1)
src/features/sicp/utils/SicpUtils.ts (4)
readSicpLangLocalStorage(23-26)setSicpLangLocalStorage(18-21)SICP_DEF_TB_LANG(15-15)readSicpSectionLocalStorage(10-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint (test-coveralls)
- GitHub Check: lint (test --bail=1)
🔇 Additional comments (2)
src/pages/sicp/Sicp.tsx (2)
107-123: Confirm intended URL canonicalization (removing thelangsegment after reading).Current logic reads the language from the URL, saves it, then navigates to a URL without the language segment. If the goal is shareable, language-stable URLs, consider preserving it:
- With lang param present: navigate(
/sicpjs/${paramLang}/${section ?? readSicpSectionLocalStorage()}, { replace: true })- Otherwise, keep current behavior.
Would you like to keep URLs canonical without the lang prefix, or preserve the language in the URL for shareability?
41-51: Good abstraction:loadInitialLangreduces first-render churn.This addresses earlier feedback about separating LS/default logic from render and avoids an extra re-render on mount.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/routes/routerConfig.tsx (1)
52-54: Route ordering and param name mismatch block language-aware routePlace the more specific two-segment route before the generic one, and align the param name with the component’s
useParams(paramLang).- { path: 'sicpjs/:section?', lazy: Sicp }, - { path: 'sicpjs/:param_lang/:section?', lazy: Sicp }, + { path: 'sicpjs/:paramLang/:section?', lazy: Sicp }, + { path: 'sicpjs/:section?', lazy: Sicp },src/pages/sicp/subcomponents/SicpToc.tsx (2)
28-31: Fix invalid TypeScript typeinteger[]→number[]This won’t compile; use
number[].- const handleNodeExpand = (_node: TreeNodeInfo, path: integer[]) => { + const handleNodeExpand = (_node: TreeNodeInfo, path: number[]) => { ... - const handleNodeCollapse = (_node: TreeNodeInfo, path: integer[]) => { + const handleNodeCollapse = (_node: TreeNodeInfo, path: number[]) => {Also applies to: 34-38
75-94: Reset error state on retry/success to avoid permanent fallbackOnce
erroris set, the fallback is always shown. Reset it before fetch or on success.React.useEffect(() => { setLoading(true); + setError(false); fetch(baseUrl + lang + '/toc.json') .then(response => { if (!response.ok) { throw Error(response.statusText); } return response.json(); }) .then(json => { setToc(json as TreeNodeInfo[]); + setError(false); }) .catch(error => { - console.log(error); + console.error(error); setError(true); }) .finally(() => { setLoading(false); }); }, [lang]);src/pages/sicp/Sicp.tsx (1)
57-59: Critical: route param name mismatch breaks language parsingIf the router keeps
:param_lang,paramLanghere will always be undefined, causing wrong language to load. Either rename the route param to:paramLang(recommended; see router comment) or destructure with alias:- const { paramLang, section } = useParams<{ paramLang?: SicpLang; section?: string }>(); + const { param_lang: paramLang, section } = useParams<{ param_lang?: SicpLang; section?: string }>();Apply only if the router still uses
:param_lang.Also applies to: 105-122
🧹 Nitpick comments (2)
src/features/sicp/utils/SicpUtils.ts (1)
15-26: Type-safety and event-name constant for resilienceAdd explicit return/param types and centralize the event name to avoid typos and ease reuse.
export const SICP_DEF_TB_LANG = 'en'; export const SICP_TB_LANG_KEY = 'sicp-textbook-lang'; +export const SICP_TB_LANG_CHANGE_EVENT = 'sicp-tb-lang-change'; + -export const setSicpLangLocalStorage = (value: string) => { - setLocalStorage(SICP_TB_LANG_KEY, value); - window.dispatchEvent(new Event('sicp-tb-lang-change')); -}; +export const setSicpLangLocalStorage = (value: string): void => { + setLocalStorage(SICP_TB_LANG_KEY, value); + window.dispatchEvent(new Event(SICP_TB_LANG_CHANGE_EVENT)); +}; -export const readSicpLangLocalStorage = () => { - const data = readLocalStorage(SICP_TB_LANG_KEY, SICP_DEF_TB_LANG); - return data; -}; +export const readSicpLangLocalStorage = (): string => { + const data = readLocalStorage(SICP_TB_LANG_KEY, SICP_DEF_TB_LANG); + return data as string; +};src/pages/sicp/Sicp.tsx (1)
204-208: Minor UX/accessibility polish for the language toggleAdd ARIA attributes and avoid hardcoding mixed-language labels.
- const handleLanguageToggle = () => { - const newLang = lang === 'en' ? 'zh_CN' : 'en'; + const handleLanguageToggle = () => { + const newLang: SicpLang = lang === 'en' ? 'zh_CN' : 'en'; setLang(newLang); setSicpLangLocalStorage(newLang); }; ... - <Button onClick={handleLanguageToggle} intent="primary" small> - {lang === 'en' ? '切换到中文' : 'Switch to English'} + <Button + onClick={handleLanguageToggle} + intent="primary" + small + aria-pressed={lang === 'zh_CN'} + aria-label={lang === 'en' ? 'Switch to Chinese' : 'Switch to English'} + > + {lang === 'en' ? 'Switch to 中文' : '切换到 English'} </Button>Also applies to: 214-229
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/pages/sicp/subcomponents/__tests__/__snapshots__/SicpIndexPage.test.tsx.snapis excluded by!**/*.snapsrc/pages/sicp/subcomponents/__tests__/__snapshots__/SicpToc.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/features/sicp/utils/SicpUtils.ts(1 hunks)src/pages/sicp/Sicp.tsx(7 hunks)src/pages/sicp/subcomponents/SicpToc.tsx(3 hunks)src/routes/routerConfig.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/pages/sicp/subcomponents/SicpToc.tsx (1)
src/features/sicp/utils/SicpUtils.ts (1)
readSicpLangLocalStorage(23-26)
src/features/sicp/utils/SicpUtils.ts (1)
src/commons/utils/LocalStorageHelper.ts (2)
setLocalStorage(6-8)readLocalStorage(1-4)
src/pages/sicp/Sicp.tsx (1)
src/features/sicp/utils/SicpUtils.ts (4)
readSicpLangLocalStorage(23-26)setSicpLangLocalStorage(18-21)SICP_DEF_TB_LANG(15-15)readSicpSectionLocalStorage(10-13)
🔇 Additional comments (2)
src/pages/sicp/Sicp.tsx (2)
105-122: Confirm intent: redirect strips the language segmentThe redirect drops the
/:paramLangprefix after reading it. If shareable URLs should preserve language, redirect to the two-segment path instead.Option A (keep current behavior): no change.
Option B (preserve language in URL):
- if (paramLang) { - navigate(`/sicpjs/${section}`, { replace: true }); - } else { - navigate(`/sicpjs/${readSicpSectionLocalStorage()}`, { replace: true }); - } + if (paramLang && section) { + navigate(`/sicpjs/${paramLang}/${section}`, { replace: true }); + } else if (paramLang) { + navigate(`/sicpjs/${paramLang}/${readSicpSectionLocalStorage()}`, { replace: true }); + } else { + navigate(`/sicpjs/${readSicpSectionLocalStorage()}`, { replace: true }); + }Please confirm which behavior is desired.
141-145: Good guard: ensure valid language before fetchingThis prevents bad requests if
langis ever corrupted.Consider centralizing this check in a small helper to DRY across components that fetch SICP content.
Pull request was converted to draft
…nto pr/yihao03/3133
…nto pr/yihao03/3133
|
@sayomaki this should be ok now but can you review my code? Though it's still blocked by the SICP PR source-academy/sicp#1082 |
Description
updated router to read language params from url and request for the corresponding page in the backend
added button in sicp page to switch between languages
Type of change
How to test
use the updated backend that supports i18n when starting the frontend
navigate to the sicp page in the frontend and switch between languages
Checklist
Summary by CodeRabbit
New Features
Enhancements