refactor: split ThemeModeContext into multiple contexts#212
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough概览将单一组合主题上下文拆分为四个独立原始上下文（AppearanceContext、ThemeModeValueContext、BrowserPrefersContext、ThemeActionContext），添加 getAntdToken 函数和 CSS 变量注入能力，更新组件和钩子以消费新上下文结构。 更改内容
估计代码审查工作量🎯 3 (中等) | ⏱️ ~25 分钟 诗
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @Innei, 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 refactors the theme management system by decomposing a single, broad theme context into several specialized contexts. This change aims to enhance the modularity, maintainability, and performance of theme-related state by reducing unnecessary re-renders and improving component isolation. Additionally, it introduces a new utility for accessing Ant Design tokens outside of React and extends static style extraction capabilities to better support SSR environments. 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. Changelog
Activity
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 is a significant and well-executed refactoring that splits the monolithic ThemeModeContext into several fine-grained contexts. This is an excellent change for improving performance by reducing unnecessary re-renders. The changes are applied consistently throughout the codebase. Additionally, the introduction of getAntdToken and the injectTokenStyle feature for SSR are valuable additions that will help prevent theme flashing (FOUC). The inclusion of new tests and benchmarks is also commendable. I have a couple of suggestions to further improve performance and code clarity.
| export const useThemeMode = (): ThemeContextState => { | ||
| const appearance = useContext(AppearanceContext); | ||
| const themeMode = useContext(ThemeModeValueContext); | ||
| const browserPrefers = useContext(BrowserPrefersContext); | ||
| const { setAppearance, setThemeMode } = useContext(ThemeActionContext); | ||
|
|
||
| return { | ||
| appearance, | ||
| isDarkMode: appearance === 'dark', | ||
| themeMode, | ||
| setThemeMode, | ||
| setAppearance, | ||
| browserPrefers, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This hook creates a new object on every render, which can lead to unnecessary re-renders in components that consume it. To improve performance, the returned object should be memoized with useMemo. This ensures that consumers only re-render when the actual theme state values change.
Note: You'll also need to add useMemo to the import from react.
| export const useThemeMode = (): ThemeContextState => { | |
| const appearance = useContext(AppearanceContext); | |
| const themeMode = useContext(ThemeModeValueContext); | |
| const browserPrefers = useContext(BrowserPrefersContext); | |
| const { setAppearance, setThemeMode } = useContext(ThemeActionContext); | |
| return { | |
| appearance, | |
| isDarkMode: appearance === 'dark', | |
| themeMode, | |
| setThemeMode, | |
| setAppearance, | |
| browserPrefers, | |
| }; | |
| }; | |
| export const useThemeMode = (): ThemeContextState => { | |
| const appearance = useContext(AppearanceContext); | |
| const themeMode = useContext(ThemeModeValueContext); | |
| const browserPrefers = useContext(BrowserPrefersContext); | |
| const actions = useContext(ThemeActionContext); | |
| return useMemo( | |
| () => ({ | |
| appearance, | |
| isDarkMode: appearance === 'dark', | |
| themeMode, | |
| browserPrefers, | |
| ...actions, | |
| }), | |
| [appearance, themeMode, browserPrefers, actions], | |
| ); | |
| }; |
| const algoProp = !themeConfig.algorithm | ||
| ? [] | ||
| : themeConfig.algorithm instanceof Array | ||
| ? themeConfig.algorithm | ||
| : [themeConfig.algorithm]; |
There was a problem hiding this comment.
The logic to normalize themeConfig.algorithm into an array can be simplified. Using [].concat() can handle undefined, a single item, or an array gracefully, making the code more concise and readable.
const algoProp = [].concat(themeConfig.algorithm || []);
…ate management - Introduced separate contexts: AppearanceContext, ThemeModeValueContext, BrowserPrefersContext, and ThemeActionContext. - Updated related components and hooks to utilize the new context structure. - Refactored tests to align with the new context setup, ensuring default values and functionality remain intact. Signed-off-by: Innei <tukon479@gmail.com>
Innei
force-pushed
the
refactor/split-theme-context
branch
from
a962b4b to
3c2d42d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/functions/extractStaticStyle.tsx`:
- Line 147: The current line in extractStaticStyle that coerces numeric rawValue
into `${rawValue}px` causes incorrect united CSS for unitless tokens; update
extractStaticStyle to only append "px" when the token key/property requires
length units by introducing a unitless token set or a heuristic regex, e.g.,
check the token identifier (the function's key/name param) against a maintained
UNIT_LESS_TOKENS or /(lineHeight|fontWeight|opacity|zIndex)/i and if it matches
keep the numeric rawValue as-is, otherwise convert numbers to `${rawValue}px`;
modify the computation of value (which currently uses rawValue) to branch on
that check so unitless tokens like lineHeight, fontWeight, opacity, zIndex
remain unitless.
In `@tests/functions/createStyles.browser.bench.test.tsx`:
- Around line 6-14: The runBenchmark helper runs fn (which calls testing-library
render) many times without unmounting, causing DOM buildup; update runBenchmark
to call the testing-library cleanup after each iteration (e.g., call cleanup()
in a finally block after invoking fn) so each render is unmounted before the
next iteration and wrap fn invocation in try/finally to ensure cleanup always
runs; reference the runBenchmark function and the fn parameter (and tests that
call render) when making the change.
In `@tests/functions/extractStaticStyle.test.tsx`:
- Around line 76-78: The assertions for tokenStyle are in the wrong order: call
expect(tokenStyle).toBeDefined() before accessing tokenStyle.css and calling
expect(tokenStyle.css).toMatchSnapshot(); update the test in
extractStaticStyle.test.tsx to first assert the variable is defined (for
tokenStyle found via result.find(...)) and then run the snapshot assertion, and
apply the same swap for the other identical occurrences around the later
assertions (the block at the same scope as the lines flagged) so you never call
.css on a potentially undefined value.
🧹 Nitpick comments (10)
tests/functions/createStyles.browser.bench.test.tsx (1)
193-340: "性能汇总"测试与前面 6 个独立测试存在大量重复代码
benchmarks数组中的每个 lambda 都是对应独立测试的复制粘贴。如果某个样式场景需要修改，必须同步更新两处，容易遗漏。建议将每个场景的
createStyles+App提取为共享工厂函数，在独立测试和汇总测试中复用。tests/functions/createStyles.bench.tsx (1)
1-158: 基准测试结构清晰，覆盖了主要的createStyles使用模式与
createStyles.browser.bench.test.tsx类似，每次bench回调中的render()没有执行cleanup()。虽然 vitest bench 的迭代管理可能减轻影响，但仍建议在每次render()后清理 DOM，以确保各次迭代的测量条件一致。src/factories/createStaticStyles/cssVar.ts (1)
42-46: 建议使用Array.isArray()代替instanceof Array
instanceof Array在跨 iframe/realm 场景下可能返回false。Array.isArray()是更可靠的检测方式。♻️ 建议修改
const algoProp = !themeConfig.algorithm ? [] - : themeConfig.algorithm instanceof Array + : Array.isArray(themeConfig.algorithm) ? themeConfig.algorithm : [themeConfig.algorithm];tests/functions/getAntdToken.test.ts (1)
1-29: 测试覆盖了核心场景，结构清晰建议后续补充：直接传入
ThemeConfig对象（非函数）的测试，以及algorithm合并逻辑的测试。src/context/ThemeModeContext.test.ts (1)
26-33: 测试名称与实际行为不一致测试名
"should return false when window is undefined"具有误导性 —— 在 JSDOM 环境中window是已定义的。此测试实际验证的是：BrowserPrefersContext的默认值在模块加载时已确定为'light'，后续渲染不会再次调用matchMedia。建议更新测试名称以准确描述行为，例如
"should use module-level default without re-calling matchMedia"。src/hooks/useThemeMode.ts (1)
11-25: 每次调用都返回新对象，考虑是否需要 memoize。
useThemeMode每次调用都返回一个新的对象字面量。如果有消费者将整个返回值传入依赖数组或作为 prop 传递，可能导致不必要的重渲染。可以考虑用useMemo包裹返回值。不过如果消费者都是解构使用，当前写法也是可接受的。
♻️ 可选优化：memoize 返回值
+import { useContext, useMemo } from 'react'; -import { useContext } from 'react'; export const useThemeMode = (): ThemeContextState => { const appearance = useContext(AppearanceContext); const themeMode = useContext(ThemeModeValueContext); const browserPrefers = useContext(BrowserPrefersContext); const { setAppearance, setThemeMode } = useContext(ThemeActionContext); - return { + return useMemo(() => ({ appearance, isDarkMode: appearance === 'dark', themeMode, setThemeMode, setAppearance, browserPrefers, - }; + }), [appearance, themeMode, browserPrefers, setAppearance, setThemeMode]); };src/functions/extractStaticStyle.tsx (3)
103-116:isColorLike未覆盖现代 CSS 颜色函数格式。当前检查遗漏了
oklch()、oklab()、lab()、lch()、hwb()、color()等现代 CSS 颜色格式。虽然目前 antd token 可能不使用这些格式，但如果用户通过自定义theme传入包含这些格式的值，filter: 'colors'模式下会将其过滤掉。
118-124:isColorTokenKey的正则匹配范围偏广。
/^[a-z]+(\d{1,2})$/i会匹配任何 "字母+1~2位数字" 的 key（如fontSize12、lineHeight15等非调色板 key）。虽然注释中说明了有意保持宽泛，但在filter: 'color'模式下可能会意外包含非颜色 token。不过在filter: 'color'模式下，由于还要通过isColorTokenKey的key.startsWith('color')分支，这里的额外匹配只影响调色板类 key 的识别，风险可控。
160-165:dangerouslySetInnerHTML使用说明。静态分析工具标记了此处的
dangerouslySetInnerHTML。在当前场景中，CSS 内容来源于 antd token 系统的键值对拼接，非用户直接输入，XSS 风险较低。但需注意，如果用户通过theme选项传入了自定义 token 值，理论上可以注入</style>等恶意内容来逃逸样式标签。此文件中已有的其他dangerouslySetInnerHTML用法（如 antd 样式和 emotion 样式）也存在相同模式，因此保持一致性是合理的。tests/functions/extractStaticStyle.test.tsx (1)
61-98: 测试覆盖整体良好，建议补充filter: 'all'和injectTokenStyle: true（布尔值）的测试用例。当前测试覆盖了
appearance指定、filter: 'color'的明暗两种模式，但缺少对filter: 'all'（包含所有 token）和直接传injectTokenStyle: true（布尔值，使用全部默认参数）的测试路径。这些是createTokenVarStyleItem的重要分支。
| if (rawValue === null || typeof rawValue === 'undefined') continue; | ||
|
|
||
| const kebabKey = toKebabCase(rawKey); | ||
| const value = typeof rawValue === 'number' ? `${rawValue}px` : String(rawValue); |
There was a problem hiding this comment.
数值类型 token 统一加 px 后缀会导致无单位属性值错误。
antd 的 design token 中存在大量无单位的数值 token，例如 lineHeight（如 1.5）、fontWeightStrong（如 600）、opacityLoading（如 0.65）、zIndexPopupBase 等。将它们统一转为 ${value}px 会产生错误的 CSS 变量值（如 --ant-line-height: 1.5px），在实际使用时将导致样式异常。
建议维护一个无单位 token key 的集合，或者采用启发式方法（如检查 key 是否包含 lineHeight、fontWeight、zIndex、opacity 等关键字）来决定是否添加 px 后缀。
🐛 建议的修复方案
+const UNITLESS_TOKEN_PATTERNS = [
+ 'lineHeight', 'fontWeight', 'opacity', 'zIndex', 'motion',
+];
+
+const isUnitlessToken = (key: string): boolean =>
+ UNITLESS_TOKEN_PATTERNS.some((pattern) => key.toLowerCase().includes(pattern.toLowerCase()));
+
// inside createTokenVarStyleItem, line 147:
- const value = typeof rawValue === 'number' ? `${rawValue}px` : String(rawValue);
+ const value = typeof rawValue === 'number'
+ ? (isUnitlessToken(rawKey) ? String(rawValue) : `${rawValue}px`)
+ : String(rawValue);📝 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 value = typeof rawValue === 'number' ? `${rawValue}px` : String(rawValue); | |
| const UNITLESS_TOKEN_PATTERNS = [ | |
| 'lineHeight', 'fontWeight', 'opacity', 'zIndex', 'motion', | |
| ]; | |
| const isUnitlessToken = (key: string): boolean => | |
| UNITLESS_TOKEN_PATTERNS.some((pattern) => key.toLowerCase().includes(pattern.toLowerCase())); | |
| const value = typeof rawValue === 'number' | |
| ? (isUnitlessToken(rawKey) ? String(rawValue) : `${rawValue}px`) | |
| : String(rawValue); |
🤖 Prompt for AI Agents
In `@src/functions/extractStaticStyle.tsx` at line 147, The current line in
extractStaticStyle that coerces numeric rawValue into `${rawValue}px` causes
incorrect united CSS for unitless tokens; update extractStaticStyle to only
append "px" when the token key/property requires length units by introducing a
unitless token set or a heuristic regex, e.g., check the token identifier (the
function's key/name param) against a maintained UNIT_LESS_TOKENS or
/(lineHeight|fontWeight|opacity|zIndex)/i and if it matches keep the numeric
rawValue as-is, otherwise convert numbers to `${rawValue}px`; modify the
computation of value (which currently uses rawValue) to branch on that check so
unitless tokens like lineHeight, fontWeight, opacity, zIndex remain unitless.
| const runBenchmark = (name: string, fn: () => void, iterations: number = 1000) => { | ||
| const start = performance.now(); | ||
| for (let i = 0; i < iterations; i++) { | ||
| fn(); | ||
| } | ||
| const end = performance.now(); | ||
| const avgTime = (end - start) / iterations; | ||
| return { name, avgTime, totalTime: end - start }; | ||
| }; |
There was a problem hiding this comment.
render() 在循环中缺少 cleanup，可能导致内存泄漏和基准结果失真
@testing-library/react 的 render 会在 DOM 中累积挂载的组件。在 1000 次迭代中不调用 cleanup()，会导致 DOM 节点持续增长，使后续迭代越来越慢，基准结果不准确。
🔧 建议修复
+import { render, cleanup } from '@testing-library/react';
-import { render } from '@testing-library/react'; const runBenchmark = (name: string, fn: () => void, iterations: number = 1000) => {
const start = performance.now();
for (let i = 0; i < iterations; i++) {
fn();
+ cleanup();
}🤖 Prompt for AI Agents
In `@tests/functions/createStyles.browser.bench.test.tsx` around lines 6 - 14, The
runBenchmark helper runs fn (which calls testing-library render) many times
without unmounting, causing DOM buildup; update runBenchmark to call the
testing-library cleanup after each iteration (e.g., call cleanup() in a finally
block after invoking fn) so each render is unmounted before the next iteration
and wrap fn invocation in try/finally to ensure cleanup always runs; reference
the runBenchmark function and the fn parameter (and tests that call render) when
making the change.
| const tokenStyle = result.find((i) => i.key === 'ant-token-dark')!; | ||
| expect(tokenStyle.css).toMatchSnapshot(); | ||
| expect(tokenStyle).toBeDefined(); |
There was a problem hiding this comment.
断言顺序应调整：先验证 toBeDefined() 再做快照匹配。
当前代码在第 77 行先调用 toMatchSnapshot()，然后在第 78 行才断言 toBeDefined()。如果 tokenStyle 为 undefined，快照会静默匹配 undefined 而不会报错，导致测试通过但实际未验证任何有意义的内容。第 92-94 行存在相同问题。
🔧 建议的修复
const tokenStyle = result.find((i) => i.key === 'ant-token-dark')!;
- expect(tokenStyle.css).toMatchSnapshot();
expect(tokenStyle).toBeDefined();
+ expect(tokenStyle.css).toMatchSnapshot();
expect(tokenStyle.css).toMatch(/--ant-color-bg-base:/);第 92-94 行同理：
const tokenStyle = result.find((i) => i.key === 'ant-token-light')!;
- expect(tokenStyle.css).toMatchSnapshot();
expect(tokenStyle).toBeDefined();
+ expect(tokenStyle.css).toMatchSnapshot();
expect(tokenStyle.css).toMatch(/--ant-color-bg-base:/);📝 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 tokenStyle = result.find((i) => i.key === 'ant-token-dark')!; | |
| expect(tokenStyle.css).toMatchSnapshot(); | |
| expect(tokenStyle).toBeDefined(); | |
| const tokenStyle = result.find((i) => i.key === 'ant-token-dark')!; | |
| expect(tokenStyle).toBeDefined(); | |
| expect(tokenStyle.css).toMatchSnapshot(); | |
| expect(tokenStyle.css).toMatch(/--ant-color-bg-base:/); |
🤖 Prompt for AI Agents
In `@tests/functions/extractStaticStyle.test.tsx` around lines 76 - 78, The
assertions for tokenStyle are in the wrong order: call
expect(tokenStyle).toBeDefined() before accessing tokenStyle.css and calling
expect(tokenStyle.css).toMatchSnapshot(); update the test in
extractStaticStyle.test.tsx to first assert the variable is defined (for
tokenStyle found via result.find(...)) and then run the snapshot assertion, and
apply the same swap for the other identical occurrences around the later
assertions (the block at the same scope as the lines flagged) so you never call
.css on a potentially undefined value.
1 participant
Split ThemeModeContext into separate contexts for better state management:
This improves component isolation and state management.
Summary by CodeRabbit
发布说明
新功能
getAntdToken()函数，支持在不依赖 React 的情况下获取 Ant Design 设计令牌。extractStaticStyle()支持injectTokenStyle选项以注入 Ant Design 令牌 CSS 变量。重构
测试