[codex] fix settings page error fallback - #1291
Conversation
github-actions
Bot
added
documentation
🤖 自动审查报告
📁 修改的文件
🧠 AI 代码审查意见结论: 结构化审查结果
必改项 (0 条)无。 建议项 (1 条)
|
There was a problem hiding this comment.
Pull request overview
This PR addresses the /settings blank/black-screen failure mode by adding a reusable React page-level error boundary and wrapping the settings route so runtime render exceptions show a diagnostic fallback (including a desktop log collection hint) instead of collapsing the page.
Changes:
- Added
PageErrorBoundarycomponent that renders a user-facing fallback with optional diagnostics and a reload action. - Wrapped the
/settingsroute inApp.tsxwithPageErrorBoundaryand provided desktop-specific support hints. - Added unit tests for the new error boundary and documented the change in
docs/CHANGELOG.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/CHANGELOG.md | Adds an Unreleased fix entry describing the settings-page error boundary. |
| apps/dsa-web/src/components/common/PageErrorBoundary.tsx | Introduces a reusable page-level React error boundary with fallback UI and reload/reset behavior. |
| apps/dsa-web/src/components/common/index.ts | Re-exports PageErrorBoundary from the common components barrel. |
| apps/dsa-web/src/components/common/tests/PageErrorBoundary.test.tsx | Adds tests verifying normal rendering and fallback rendering/reset callback behavior. |
| apps/dsa-web/src/App.tsx | Wraps the /settings route with PageErrorBoundary and provides diagnostic/support text. |
| private handleReset = () => { | ||
| if (this.props.onReset) { | ||
| this.props.onReset(); | ||
| return; | ||
| } | ||
| window.location.reload(); | ||
| }; |
ZhuLinsen
left a comment
ZhuLinsen
left a comment
There was a problem hiding this comment.
评审结论
- 必要性：通过，关联 Windows 桌面端设置页黑屏/空白页诊断问题，增加路由级错误边界有实际修复价值。
- 是否有对应 issue：有，Fixes #1287。
- PR 类型：fix，主要修复
/settings运行时异常导致页面崩溃为黑屏的问题。 - description 完整性：完整，已包含 Summary、Root Cause、Validation、Risk 和 Rollback，且说明了未完全证明原 Windows portable 根因。
- 是否可直接合入：可，当前 CI 为 success，改动范围聚焦 Web 设置页错误兜底与测试；
mergeable_state=blocked更像分支保护状态，不单独构成阻断。
主要问题
- [Nice to have]
apps/dsa-web/src/components/common/PageErrorBoundary.tsx新增了onReset可选 API，但handleReset在调用onReset后不会清空hasError/message，因此后续复用该组件并传入onReset时，fallback 可能一直停留在错误状态，除非父组件强制卸载或改 key。当前/settings路径未传onReset，主修复路径不受影响；建议补充清状态逻辑或移除暂未使用的 API，并把测试从“只验证回调被调用”扩展到“reset 后可恢复渲染”的行为验证。
🤖 此回复由 OpenReview Bot 自动生成，仅供参考。如有疑问请 @维护者。
6441481 to
a1c350d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1c350d47d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| {this.state.message ? ( | ||
| <pre className="max-h-32 overflow-auto rounded-xl border border-danger/20 bg-danger/5 px-3 py-2 text-xs leading-5 text-danger"> | ||
| {this.state.message} |
There was a problem hiding this comment.
When a settings child outside the existing panel boundary throws an Error whose message includes config-derived values such as a webhook URL, Authorization header, or API key, this page-level fallback renders that message verbatim in the UI. The existing SettingsPanelErrorBoundary already sanitizes and truncates these summaries, but this new boundary catches broader settings-page failures and can expose secrets in screenshots/support reports; please apply the same redaction/truncation before displaying the message.
Useful? React with 👍 / 👎.
Summary
Fixes #1287.
/settingsso settings runtime errors render a diagnostic fallback instead of a blank page.logs/desktop.logand the failing settings category.Root Cause
The original Windows portable black-screen report still needs release/version logs to identify the exact failing settings branch. However, the settings route had no page error boundary, so any runtime render exception could collapse the page into a blank state.
Validation
cd apps/dsa-web && npm cicd apps/dsa-web && npm run test -- PageErrorBoundary.test.tsx(2 passed)cd apps/dsa-web && npm run lintcd apps/dsa-web && npm run buildNote:
npm cireported existing dependency audit findings: 10 vulnerabilities (4 moderate, 6 high). No new dependencies were added.Risk
This prevents black-screen UX and improves diagnostics, but it does not prove the original Windows portable root cause. The follow-up still needs release version and
desktop.logfrom an affected machine.Rollback
Revert this PR to remove the settings page error boundary.