feat: 实施性能优化、UX主题、高危操作二次确认及DX规范#54
Conversation
✅ Deploy Preview for pushy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughChangesThe PR adds bundle analysis and experience-based chunk splitting, reusable lazy chart and skeleton/error handling, asynchronous JSON editor loading, and a text-matching destructive-action confirmation flow with localized strings and tests. Performance loading changes
Destructive action confirmation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MetricsPage
participant AsyncLine
participant SectionErrorBoundary
participant Suspense
participant AntDesignCharts
MetricsPage->>AsyncLine: Render chart props
AsyncLine->>SectionErrorBoundary: Render lazy chart boundary
SectionErrorBoundary->>Suspense: Render chart with fallback
Suspense->>AntDesignCharts: Dynamically import Line
Suspense-->>MetricsPage: Show ChartSkeleton while loading
AntDesignCharts-->>AsyncLine: Provide Line module
AsyncLine-->>MetricsPage: Render chart
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsbuild.config.ts`:
- Around line 21-30: Update the performance.chunkSplit configuration in
rsbuild.config.ts to remove the forceSplitting entries for charts, jsoneditor,
and wasm so lazy-route dependencies are not injected into the initial HTML. If
vendor grouping is still required, recreate it through splitChunks instead while
preserving lazy loading.
In `@src/components/lazy-chart.tsx`:
- Around line 6-20: Update the LazyArea, LazyLine, LazyPie, and LazyDualAxes
loaders so their lazy wrappers and import promises are recreated when
SectionErrorBoundary invokes onReset, rather than remaining module-level
singletons. Wire the reset callback through the lazy-chart component and
preserve the existing chart component mappings while ensuring “重试组件” mounts a
newly created loader.
In `@src/pages/manage/components/json-editor.tsx`:
- Around line 53-64: Update the loading spinner container in the JSON editor
wrapper to use absolute positioning with inset 0, plus an appropriate background
and z-index, so it overlays and centers over the editor without consuming layout
space. Keep the wrapper’s existing relative positioning and loading behavior
unchanged.
- Around line 18-28: Update the editor initialization around createJSONEditor to
store the latest props in a ref and read that ref when the dynamic import
resolves, rather than using the effect’s captured props. Ensure initialization
uses current content, callbacks, and options so prop changes occurring before
refEditor.current exists are not missed.
- Around line 31-34: Update the vanilla-jsoneditor import failure handler in the
JSON editor component to set a user-facing error state when loading fails, while
preserving the existing loading-state cleanup. Render that error state in the
editor area with an actionable retry path, ensuring failures are visible to
flows using JsonEditor from version-table.tsx and deps-table.tsx.
In `@src/pages/manage/components/setting-modal.tsx`:
- Around line 17-21: Update the deletion confirmation flow around
confirmTargetText to use only a non-empty appName, removing the appKey and appId
fallbacks. Ensure deletion remains disabled or blocked when appName is unset or
cleared, and preserve the existing name-matching confirmation behavior when it
is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4588789f-6c65-4957-8ee8-ab596801218c
📒 Files selected for processing (11)
package.jsonrsbuild.config.tssrc/components/dangerous-confirm-modal.tsxsrc/components/lazy-chart.tsxsrc/components/section-error-boundary.tsxsrc/components/skeletons.tsxsrc/pages/admin-metrics.tsxsrc/pages/manage/components/json-editor.tsxsrc/pages/manage/components/setting-modal.tsxsrc/pages/realtime-metrics.tsxsrc/pages/version-health.tsx
…r handling, and security modal
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/lazy-chart.tsx`:
- Around line 57-119: Update AsyncArea, AsyncLine, AsyncPie, and AsyncDualAxes
so the destructured height value is included in chartProps passed to
AsyncChartWrapper, allowing the underlying chart to receive explicit heights
while preserving the existing loading height behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f769c026-0599-4f7a-9a86-b7d8a053498f
📒 Files selected for processing (5)
rsbuild.config.tssrc/components/dangerous-confirm-modal.tsxsrc/components/lazy-chart.tsxsrc/pages/manage/components/json-editor.tsxsrc/pages/manage/components/setting-modal.tsx
💤 Files with no reviewable changes (1)
- rsbuild.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/manage/components/setting-modal.tsx
- src/components/dangerous-confirm-modal.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rsbuild.config.ts (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigrate from the deprecated
performance.chunkSplitAPI.This project uses Rsbuild 2.1.5, where
performance.chunkSplitis deprecated. Move this configuration to top-levelsplitChunks(for example,preset: 'default') and verify that the generated chunks preserve the intended lazy-loading boundaries. (rsbuild.rs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsbuild.config.ts` around lines 22 - 24, Replace the deprecated performance.chunkSplit configuration in the Rsbuild configuration with the top-level splitChunks API, preserving the intended chunk-splitting behavior through the appropriate preset or equivalent settings. Verify that generated chunks retain the existing lazy-loading boundaries.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsbuild.config.ts`:
- Around line 27-38: Update the tools.rspack callback to return the mutated
config after the conditional BundleAnalyzerPlugin insertion, preserving the
existing configuration for normal builds and build:analyze.
In `@src/components/section-error-boundary.test.tsx`:
- Around line 42-57: The retry test should verify recovery, not only the
callback. Update the onReset handling around SectionErrorBoundary to make
ProblemChild stop throwing after reset, then assert that “Normal Content” is
rendered after clicking the “重试组件” button while preserving the existing onReset
call-count assertion.
- Around line 27-28: Restore console.error reliably in the section error
boundary tests by moving the mock setup and test execution under try/finally or
centralizing restoration in afterEach. Ensure every affected test restores the
captured originalError even when rendering, assertions, or locator lookups
throw.
In `@src/pages/manage/components/setting-modal.tsx`:
- Around line 21-22: Update confirmTargetText in the setting-modal confirmation
flow to use only the trimmed appName, without falling back to appKey. Ensure the
destructive delete action remains disabled or blocked while appName is empty or
unavailable, and preserve exact application-name matching once it is present.
---
Nitpick comments:
In `@rsbuild.config.ts`:
- Around line 22-24: Replace the deprecated performance.chunkSplit configuration
in the Rsbuild configuration with the top-level splitChunks API, preserving the
intended chunk-splitting behavior through the appropriate preset or equivalent
settings. Verify that generated chunks retain the existing lazy-loading
boundaries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 500a65fe-97d7-4783-b43c-1e42d995e802
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
bun-test-setup.tspackage.jsonrsbuild.config.tssrc/components/dangerous-confirm-modal.test.tsxsrc/components/dangerous-confirm-modal.tsxsrc/components/section-error-boundary.test.tsxsrc/components/section-error-boundary.tsxsrc/i18n/locales/en.jsonsrc/i18n/locales/zh-CN.jsonsrc/pages/manage/components/setting-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/section-error-boundary.tsx
- src/components/dangerous-confirm-modal.tsx
| tools: { | ||
| rspack: (config) => { | ||
| if (process.env.BUNDLE_ANALYZE === 'true') { | ||
| config.plugins?.push( | ||
| new BundleAnalyzerPlugin({ | ||
| analyzerMode: 'static', | ||
| openAnalyzer: false, | ||
| }) as any, | ||
| ); | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)rsbuild\.config\.(ts|js|mts|mjs|cts|cjs)$|package\.json|pnpm-lock\.yaml|package-lock\.json|yarn\.lock' || true
echo
echo "== rsbuild config =="
if [ -f rsbuild.config.ts ]; then
cat -n rsbuild.config.ts
fi
echo
echo "== dependencies mentioning rsbuild/rspack/bundle-analyzer =="
if [ -f package.json ]; then
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
for (const sec of ['dependencies','devDependencies','peerDependencies','optionalDependencies']) {
if (pkg[sec]) {
for (const [k,v] of Object.entries(pkg[sec])) {
if (/rsbuild|rspack|analyze|bundle-analyzer/i.test(k) || /analyze|bundle-analyzer|rsbuild|rspack/i.test(v)) {
console.log(`${sec}.${k}=${v}`);
}
}
}
}
JS
fi
echo
echo "== package lock snippets if present =="
for f in pnpm-lock.yaml package-lock.json yarn.lock; do
if [ -f "$f" ]; then
echo "-- $f --"
rg -n "rsbuild|rspack|webpack-bundle-analyzer|`@rsbuild`|bundle-analyzer" "$f" | head -80 || true
fi
done
echo
echo "== documentation references in repo =="
rg -n "tools\.rspack|rspack:\s*\(.*config\)|return config|BundleAnalyzerPlugin|analyze" . --glob '!node_modules' --glob '!dist' --glob '!build' | head -120 || trueRepository: reactnativecn/pushy-admin
Length of output: 3494
🌐 Web query:
rsbuild tools rspack config function return config documentation
💡 Result:
In Rsbuild, the tools.rspack configuration option allows you to modify the underlying Rspack configuration object [1][2]. When you define tools.rspack as a function, you receive the built-in Rspack configuration object as the first parameter, which you can then modify and return [1][3]. Key details regarding this function: 1. Final Configuration: The object returned by your function is used directly as the final Rspack configuration and is not automatically merged with the built-in Rsbuild Rspack configuration [1][3]. 2. Modification and Merging: If you intend to merge your custom configuration with the built-in one rather than overriding it, you can use the mergeConfig utility provided in the second argument of the function [1][3]. Example using modification: export default { tools: { rspack: (config) => { config.resolve.alias ||= {}; config.resolve.alias['@util'] = 'src/util'; return config; }, }, }; Example using mergeConfig: export default { tools: { rspack: (config, { mergeConfig }) => { return mergeConfig(config, { devtool: 'eval', }); }, }, }; The second argument provided to the function also includes utilities such as environment information (e.g., env) [2][4]. For more complex modifications, Rsbuild also supports tools.bundlerChain, which provides a chaining API for modifying the configuration [2][5].
Citations:
- 1: https://rsbuild.rs/config/tools/rspack
- 2: https://rsbuild.rs/guide/configuration/rspack
- 3: https://v2.rsbuild.rs/config/tools/rspack
- 4: https://v2.rsbuild.rs/guide/configuration/rspack
- 5: https://rsbuild.rs/guide/configuration/rspack.md
Return the Rspack config from the hook.
The function-form tools.rspack receiver replaces Rsbuild’s built-in Rspack config with the value returned from the callback. Returning undefined replaces the configuration and can break normal builds and build:analyze, so return the mutated config before closing the hook.
Proposed fix
if (process.env.BUNDLE_ANALYZE === 'true') {
config.plugins?.push(
new BundleAnalyzerPlugin({
analyzerMode: 'static',
openAnalyzer: false,
}) as any,
);
}
+ return config;
},📝 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.
| tools: { | |
| rspack: (config) => { | |
| if (process.env.BUNDLE_ANALYZE === 'true') { | |
| config.plugins?.push( | |
| new BundleAnalyzerPlugin({ | |
| analyzerMode: 'static', | |
| openAnalyzer: false, | |
| }) as any, | |
| ); | |
| } | |
| }, | |
| }, | |
| tools: { | |
| rspack: (config) => { | |
| if (process.env.BUNDLE_ANALYZE === 'true') { | |
| config.plugins?.push( | |
| new BundleAnalyzerPlugin({ | |
| analyzerMode: 'static', | |
| openAnalyzer: false, | |
| }) as any, | |
| ); | |
| } | |
| return config; | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsbuild.config.ts` around lines 27 - 38, Update the tools.rspack callback to
return the mutated config after the conditional BundleAnalyzerPlugin insertion,
preserving the existing configuration for normal builds and build:analyze.
Source: MCP tools
| const originalError = console.error; | ||
| console.error = () => {}; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore console.error in teardown.
If a render, assertion, or locator lookup fails, execution skips the restoration lines and leaves console.error mocked for subsequent tests. Use try/finally or restore the mock in afterEach.
Also applies to: 39-39, 43-44, 59-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/section-error-boundary.test.tsx` around lines 27 - 28, Restore
console.error reliably in the section error boundary tests by moving the mock
setup and test execution under try/finally or centralizing restoration in
afterEach. Ensure every affected test restores the captured originalError even
when rendering, assertions, or locator lookups throw.
| test('invokes onReset and allows retry when retry button is clicked', () => { | ||
| const originalError = console.error; | ||
| console.error = () => {}; | ||
|
|
||
| const onResetMock = mock(() => {}); | ||
|
|
||
| render( | ||
| <SectionErrorBoundary title="Section Error" onReset={onResetMock}> | ||
| <ProblemChild shouldThrow={true} /> | ||
| </SectionErrorBoundary>, | ||
| ); | ||
|
|
||
| const retryBtn = screen.getByText('重试组件').closest('button')!; | ||
| fireEvent.click(retryBtn); | ||
|
|
||
| expect(onResetMock).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the retry test verify a successful retry.
ProblemChild always throws because shouldThrow remains true, so the test only verifies onReset. It would also pass if the boundary failed to clear its error state. Toggle the child to non-throwing state in onReset, then assert that Normal Content is rendered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/section-error-boundary.test.tsx` around lines 42 - 57, The
retry test should verify recovery, not only the callback. Update the onReset
handling around SectionErrorBoundary to make ProblemChild stop throwing after
reset, then assert that “Normal Content” is rendered after clicking the “重试组件”
button while preserving the existing onReset call-count assertion.
| // 优先使用 appName，如尚未设定或加载中时使用稳定的 appKey 作为二次确认识别串 | ||
| const confirmTargetText = appName?.trim() || appKey?.trim() || ''; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not fall back to appKey for destructive confirmation.
When appName is empty or cleared, this fallback allows deletion by typing the app key, violating the required exact application-name match. Use only a non-empty trimmed appName, and disable/block the delete action until it is available. This previously reported issue is still present in the current code.
Proposed fix
- const confirmTargetText = appName?.trim() || appKey?.trim() || '';
+ const confirmTargetText = appName?.trim() || '';
<Button
...
+ disabled={!confirmTargetText}
onClick={() => setShowConfirmModal(true)}
>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/manage/components/setting-modal.tsx` around lines 21 - 22, Update
confirmTargetText in the setting-modal confirmation flow to use only the trimmed
appName, without falling back to appKey. Ensure the destructive delete action
remains disabled or blocked while appName is empty or unavailable, and preserve
exact application-name matching once it is present.
1 participant
变更内容摘要
1. 性能与首屏加载（二）
rsbuild.config.ts中performance.chunkSplit，隔离@ant-design/charts、vanilla-jsoneditor与hash-wasmChunk。src/components/lazy-chart.tsx动态懒加载组件。admin-metrics.tsx、realtime-metrics.tsx、version-health.tsx中的同步图表引用为AsyncLine。json-editor.tsx改为异步按需加载vanilla-jsoneditor。2. 用户体验与交互设计（五）
src/components/section-error-boundary.tsx局部 Error Boundary 降级组件。src/components/skeletons.tsx规范骨架屏（TableSkeleton/ChartSkeleton/CardSkeleton）。3. 高危操作二次确认 Guard（六）
src/components/dangerous-confirm-modal.tsx高危毁灭性操作二次确认 Guard。setting-modal.tsx应用删除时接入高危二次确认弹窗，必须精准匹配应用名称方可点击删除。4. 工程化与开发者体验（八）
package.json中增加build:analyze产物体积分析指令。bun run ci/bun run lint:fix确保类型安全与 Biome 规范通过。Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Improvements
Performance/Build
Tests