feat(settings): redesign settings and agent management - #47
Merged
Conversation
📝 WalkthroughWalkthroughThe change adds profile-backed Agent settings with migration and usage synchronization, replaces the embedded Agent settings view with a unified Dao Settings page, and introduces a continuous Settings overview with new routing, search, responsive layout, localization, and validation coverage. ChangesUnified Dao settings
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
moonrailgun
force-pushed
the
moonrailgun/settings-redesign
branch
from
August 8, 2026 18:11
f91b387 to
70ef0e0
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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/patches/chrome/browser/resources/settings/dao_page/dao_page.ts.patch (1)
287-295: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win处理复制请求失败。
copyDaoMcpSetupContent()拒绝时，await会退出方法并留下未处理 rejection。如果前一次复制成功，daoMcpSetupCopied_还会继续显示成功状态。捕获错误，并在请求仍是当前请求时将
daoMcpSetupCopied_设为false。建议修改
const request = ++this.daoMcpCopyRequest_; const option = this.daoMcpSetupOption_; - const copied = - await this.daoMcpBrowserProxy_.copyDaoMcpSetupContent(option); + let copied = false; + try { + copied = + await this.daoMcpBrowserProxy_.copyDaoMcpSetupContent(option); + } catch { + copied = false; + } if (request === this.daoMcpCopyRequest_ &&🤖 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/patches/chrome/browser/resources/settings/dao_page/dao_page.ts.patch` around lines 287 - 295, Update onDaoMcpCopySetupContent_ to catch failures from daoMcpBrowserProxy_.copyDaoMcpSetupContent(option); when the request, selected option, and quick-setup visibility still match, set daoMcpSetupCopied_ to false, while preserving the existing success assignment for completed requests.
🧹 Nitpick comments (9)
src/patches/chrome/app/settings_strings.grdp.patch (2)
205-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value合并重复的英文源文本。
IDS_SETTINGS_DAO_AGENT_MODEL_TITLE与IDS_SETTINGS_DAO_AGENT_GROUP_MODEL_AND_CONNECTION的源文本都是 “Model and connection”；IDS_SETTINGS_DAO_AGENT_MANAGEMENT_TITLE与IDS_SETTINGS_DAO_AGENT_GROUP_DATA_AND_MANAGEMENT的源文本都是 “Data and management”。两组各自维护一份译文，后续文案调整需要改两处，容易出现不一致。若页面上这两处显示的是同一个标题，建议只保留分组标题消息。Also applies to: 223-225, 382-384
🤖 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/patches/chrome/app/settings_strings.grdp.patch` around lines 205 - 219, 合并重复的英文源文本消息：移除 IDS_SETTINGS_DAO_AGENT_MODEL_TITLE 和 IDS_SETTINGS_DAO_AGENT_MANAGEMENT_TITLE，保留对应的 IDS_SETTINGS_DAO_AGENT_GROUP_MODEL_AND_CONNECTION 与 IDS_SETTINGS_DAO_AGENT_GROUP_DATA_AND_MANAGEMENT 分组标题消息，并更新所有引用以使用保留的消息标识符。
193-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win删除无引用的 Configure/Unavailable 字符串
这三条字符串只在
settings_strings.grdp.patch和settings_localized_strings_provider.cc.patch中声明，没有 WebUI 使用者。若不会接入，删除 GRDP 消息、provider 注册和译文，避免遗留死字符串。🤖 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/patches/chrome/app/settings_strings.grdp.patch` around lines 193 - 201, Remove the unreferenced IDS_SETTINGS_DAO_AGENT_CONFIGURE_TITLE, IDS_SETTINGS_DAO_AGENT_CONFIGURE_DESCRIPTION, and IDS_SETTINGS_DAO_AGENT_SUMMARY_UNAVAILABLE messages from the GRDP patch, along with their registrations in settings_localized_strings_provider.cc.patch and any corresponding translations. Do not alter other Dao Agent strings or WebUI behavior.scripts/commands/__tests__/settings_redesign_contract.test.ts (1)
632-641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win移除硬编码的 177 行数断言。
真正需要保证的不变量是“hunk 头声明的行数 == 实际新增行数 ==
git apply --numstat统计值”，第 639-648 行已经覆盖。额外断言payloadCount恰好为 177 会使代理补丁的任何合法改动都导致测试失败，却不提供额外的契约保证。♻️ 建议重构
expect(hunkHeader, "new-file hunk header").not.toBeNull(); - expect(payloadCount).toBe(177); + expect(payloadCount).toBeGreaterThan(0); expect(Number(hunkHeader![1]), "declared new-file line count").toBe( payloadCount, );🤖 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 `@scripts/commands/__tests__/settings_redesign_contract.test.ts` around lines 632 - 641, Remove the hard-coded payloadCount equality assertion expecting 177 from the proxyPatch test. Keep the hunkHeader-to-payloadCount check and the existing assertions around lines 639-648 that verify the declared hunk count, actual additions, and git apply --numstat value remain equal.src/dao/browser/agent/dao_agent_settings_handler.h (1)
21-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议重命名以避免与 pref 名称常量同名。
dao::kDaoAgentSettingsMigrationVersion（int）与dao::prefs::kDaoAgentSettingsMigrationVersion（pref 键字符串）同名。两者在同一顶层命名空间树内，阅读代码时容易混淆，在namespace dao::prefs作用域内的非限定查找也会先命中 pref 常量。建议把版本号常量改为kDaoAgentSettingsSchemaVersion之类的名称。🤖 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/dao/browser/agent/dao_agent_settings_handler.h` around lines 21 - 29, Rename the top-level integer constant kDaoAgentSettingsMigrationVersion to a schema-specific name such as kDaoAgentSettingsSchemaVersion, and update every reference to it. Leave the string pref key constant in dao::prefs unchanged to avoid namespace and lookup ambiguity.src/dao/browser/agent/dao_agent_settings_handler.cc (1)
642-687: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议复用已解析的 service，减少一次工厂查找。
外层回调中重新调用
Profile::FromWebUI与DaoAgentWorkspaceServiceFactory::GetForProfile。该服务在发起GetUsageInfo时已经存在，可把它以base::WeakPtr或再次查找前的短路方式简化，降低嵌套层级。此为可选清理，不影响正确性。🤖 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/dao/browser/agent/dao_agent_settings_handler.cc` around lines 642 - 687, 在 HandleGetWorkspaceSummary 的 GetUsageInfo 回调中复用发起请求时已获取的 DaoAgentWorkspaceService，避免再次调用 Profile::FromWebUI 和 DaoAgentWorkspaceServiceFactory::GetForProfile。将该 service 传入后续回调，保持服务不存在时的现有空结果处理，并简化嵌套逻辑。src/dao/browser/ui/dao_ui_sources.gni (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value两个 GN 列表的新条目未按字典序插入。 根因相同：新增的
dao_agent_settings_handler相关条目被放在dao_agent_lock_tab_helper之后，而不是按字母顺序放在dao_agent_scenario_registry/dao_agent_proactive_*附近。gn format会重新排序原本有序的字符串列表，导致额外 diff。
src/dao/browser/ui/dao_ui_sources.gni#L8-L9：把dao_agent_settings_handler.cc与dao_agent_settings_handler.h移到dao_agent_scenario_registry.h之后、dao_agent_skill_service.cc之前。src/dao/browser/agent/BUILD.gn#L13-L13：把dao_agent_settings_handler_unittest.cc移到dao_agent_scenario_registry_unittest.cc之后、dao_agent_workspace_service_unittest.cc之前。🤖 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/dao/browser/ui/dao_ui_sources.gni` around lines 8 - 9, 按字典序调整两个 GN 列表中的新增条目：在 src/dao/browser/ui/dao_ui_sources.gni 第 8-9 行，将 dao_agent_settings_handler.cc 和 dao_agent_settings_handler.h 移到 dao_agent_scenario_registry.h 之后、dao_agent_skill_service.cc 之前；在 src/dao/browser/agent/BUILD.gn 第 13 行，将 dao_agent_settings_handler_unittest.cc 移到 dao_agent_scenario_registry_unittest.cc 之后、dao_agent_workspace_service_unittest.cc 之前，确保列表保持有序并避免 gn format 产生额外 diff。src/dao/browser/agent/dao_agent_settings_handler_unittest.cc (1)
99-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议补充整型计数的回归测试。
当前用例只覆盖 double 形式的用量数据。
dao.agent_usage_stats中若存在整型计数，RecordDaoAgentApiUsage与RecordDaoAgentToolUsage会解引用空 optional（详见src/dao/browser/agent/dao_agent_settings_handler.cc的相关评论）。请在修复后增加一个用例：先用ScopedDictPrefUpdate写入整型的完整 schema，再调用两个 Record 函数并断言累加结果。🤖 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/dao/browser/agent/dao_agent_settings_handler_unittest.cc` around lines 99 - 121, 在 DaoAgentSettingsHandlerTest 中新增整型计数回归测试：使用 ScopedDictPrefUpdate 向 prefs::kDaoAgentUsageStats 写入完整 schema 的整数值，然后调用 RecordDaoAgentApiUsage 和 RecordDaoAgentToolUsage，断言 BuildDaoAgentUsageStats 返回对应的累加结果，覆盖整数类型不会触发 optional 解引用。src/patches/chrome/browser/resources/settings/dao_page/dao_agent_settings_browser_proxy.ts.patch (1)
41-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win建议为
DaoAgentUsageStats增加运行时类型守卫。内存和工作区摘要都有
isDaoAgentMemorySummary/isDaoAgentWorkspaceSummary校验，用量统计没有。dao_agent_page.ts的updateUsageStats_在 WebUI listener 中直接展开stats.toolCalls，该路径没有 try/catch。如果原生侧发送了畸形负载，listener 会抛出异常。请添加与现有守卫风格一致的
isDaoAgentUsageStats，并在loadUsageStats_与updateUsageStats_中使用它。♻️ 建议新增守卫
+export function isDaoAgentUsageStats( + value: unknown): value is DaoAgentUsageStats { + if (!isRecord(value) || !isRecord(value['toolCalls'])) { + return false; + } + return isNonNegativeFiniteNumber(value['apiCalls']) && + isNonNegativeFiniteNumber(value['promptTokens']) && + isNonNegativeFiniteNumber(value['completionTokens']) && + isNonNegativeFiniteNumber(value['totalTokens']) && + isNonNegativeFiniteNumber(value['estimatedCost']) && + isNonNegativeFiniteNumber(value['lastReset']) && + Object.values(value['toolCalls']).every(isNonNegativeFiniteNumber); +}Also applies to: 97-98
🤖 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/patches/chrome/browser/resources/settings/dao_page/dao_agent_settings_browser_proxy.ts.patch` around lines 41 - 49, 为 DaoAgentUsageStats 增加与 isDaoAgentMemorySummary 和 isDaoAgentWorkspaceSummary 一致的运行时类型守卫 isDaoAgentUsageStats，校验所有字段及 toolCalls 的结构；在 dao_agent_page.ts 的 loadUsageStats_ 与 updateUsageStats_ 中先使用该守卫验证原生负载，只有通过校验后才更新或展开 stats.toolCalls，并安全处理无效数据以避免 listener 抛出异常。src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch (1)
465-499: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
formatCost_固定使用 USD，formatTimestamp_未处理未初始化的时间戳。两点建议：
formatCost_以currency: 'USD'格式化。数值本身是按美元计价的估算成本，货币符号应保持固定，但请确认这一点符合本地化要求。- 当用量从未重置时，原生侧的
lastReset可能为0，界面会显示 1970 年 1 月 1 日。建议在该情况下显示占位文案。🤖 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/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch` around lines 465 - 499, Keep formatCost_ fixed to USD for the dollar-denominated estimate, and update formatTimestamp_ to return the established placeholder text when value represents an uninitialized timestamp such as 0; otherwise preserve the existing localized date-time formatting.
🤖 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 `@docs/feature-checklist.md`:
- Line 105: Update the checklist entry’s Agent gear verification requirement to
use the canonical `dao://settings/#agent` anchor instead of
`dao://settings/#dao`, keeping it consistent with the documented settings
contract and `settings_redesign_contract.test.ts`.
In `@scripts/commands/__tests__/settings_redesign_contract.test.ts`:
- Line 421: Remove the leading “+” diff marker from the assertion in
settings_redesign_contract.test.ts, leaving the expect call as a normal test
statement so it passes lint.
In `@src/dao/browser/agent/dao_agent_settings_handler.cc`:
- Around line 563-590: Update HandleGetSettings, HandleMigrateLegacySettings,
and HandleSetSetting so every validation-failure or missing-GetPrefs path
invokes RejectJavascriptCallback with the supplied callback identifier before
returning. Match the existing failure-handling pattern in HandleResetUsageStats,
while preserving the current successful ResolveJavascriptCallback behavior.
- Around line 461-479: Update the migration loop in the DaoAgentSettings
migration block so each value is checked against kMaxSettingValueBytes before
adding its size to total_bytes; only valid-sized values should contribute to the
aggregate limit and proceed to update->Set.
- Around line 252-257: Update PrepareUsageStatsForUpdate to write the normalized
dictionary back into update when NormalizeUsageStats succeeds, ensuring integer
counters are converted to doubles before downstream consumers such as
RecordDaoAgentApiUsage and RecordDaoAgentToolUsage access them. Preserve the
existing NewUsageStats fallback when normalization fails.
In `@src/dao/browser/ui/webui/resources/agent/__tests__/dao_agent_app.test.ts`:
- Around line 99-108: Update the settings-button lookup in the “opens unified
settings from the settings button” test to query the light DOM root by using
el.shadowRoot when available and el as the fallback. Preserve the existing
selector and assertions, following the root-selection pattern already used by
DaoAgentApp.
In `@src/dao/browser/ui/webui/resources/agent/agent_settings_native_bridge.ts`:
- Around line 64-88: Prevent initializeAgentSettingsSync from overwriting newer
dao-agent-settings-changed events received during initialization. Track a
monotonic settings revision or equivalent ordering state alongside the existing
usageStatsGeneration, and apply the initial snapshot only when it is not older
than the latest processed settings event; add coverage for an event arriving
before initialization completes.
In `@src/dao/browser/ui/webui/resources/agent/agent_settings_sync.ts`:
- Around line 26-44: Update MANAGED_AGENT_SETTING_KEYS so
collectLegacyAgentSettings() also migrates dao_agent_memory_enabled,
dao_dream_enabled, and dao_dream_debug, then add migration tests covering
preservation of these three persisted settings.
In `@src/dao/browser/ui/webui/resources/agent/dao_agent_app.ts`:
- Around line 274-277: Replace the hardcoded title on the settings button in the
template rendering openUnifiedSettings_ with the existing Agent WebUI locale key
and i18n lookup pattern used by nearby localized views, preserving the button’s
behavior and icon.
In
`@src/patches/chrome/browser/resources/settings/a11y_page/a11y_page_index.html.patch`:
- Around line 9-13: 限定
a11y_page_index.html.patch、appearance_page_index.html.patch、autofill_page_index.html.patch、people_page_index.html.patch
和 your_saved_info_page_index.html.patch 中的 cr-view-manager
[slot=view]:not(.closing) 规则，仅匹配带有 show-all 属性的 cr-view-manager，即改为使用
cr-view-manager[show-all]，保留默认路由的定位和过渡行为。
In
`@src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.html.patch`:
- Around line 760-771: 修正 dao-agent-management-card-title 及相关活动标题的语义层级：将管理卡片标题从
h4 改为 h3，并将活动标题从 h5 改为 h4，保持 dataAndManagementHeading 的 h2 不变，确保标题层级连续。
In
`@src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch`:
- Line 124: Update agentSettingsValues_ in the Polymer element by declaring it
in static get properties() and replacing the initialized class field with
declare private agentSettingsValues_: Record<string, string>; so the generated
reactive accessor is preserved. In
src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch
lines 124-124, apply the declaration change; in
src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.html.patch
lines 639-648, retain the existing binding and add an assertion verifying an
individual tool toggle synchronizes after snapshot rerender.
In
`@src/patches/chrome/browser/resources/settings/settings_main/settings_main.ts.patch`:
- Around line 159-162: Defer the restoreOverviewScroll call in the
search-clearing flow until after the next render, rather than invoking it
synchronously after setupOverviewObserver_. Keep the !this.inSearchMode_ guard
and ensure restoreOverviewScroll reads scrollHeight only after hidden sections
have been laid out.
In
`@src/patches/chrome/browser/resources/settings/settings_ui/settings_ui.html.patch`:
- Around line 212-220: Wrap the top search area in a header element with
role="banner" around the daoSettingsNavigation aside, preserving the existing
cr-toolbar-search-field configuration and navigation structure so screen readers
retain the banner landmark.
In
`@src/patches/chrome/browser/resources/settings/settings_ui/settings_ui.ts.patch`:
- Around line 181-198: Update onSettingsSectionActivate_ to cancel
searchDebounceTimer_ before scrolling or navigating, and clear or commit the
pending search input so the field, URL, and overview remain consistent. Add a
regression test covering selecting a section immediately after entering search
text during the debounce window.
In `@src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch`:
- Around line 530-532: Update the four assertions using refreshError or
actionError textContent at the referenced locations to explicitly handle the
nullable textContent value before trimming, while preserving the existing string
comparison behavior and expected error messages.
---
Outside diff comments:
In `@src/patches/chrome/browser/resources/settings/dao_page/dao_page.ts.patch`:
- Around line 287-295: Update onDaoMcpCopySetupContent_ to catch failures from
daoMcpBrowserProxy_.copyDaoMcpSetupContent(option); when the request, selected
option, and quick-setup visibility still match, set daoMcpSetupCopied_ to false,
while preserving the existing success assignment for completed requests.
---
Nitpick comments:
In `@scripts/commands/__tests__/settings_redesign_contract.test.ts`:
- Around line 632-641: Remove the hard-coded payloadCount equality assertion
expecting 177 from the proxyPatch test. Keep the hunkHeader-to-payloadCount
check and the existing assertions around lines 639-648 that verify the declared
hunk count, actual additions, and git apply --numstat value remain equal.
In `@src/dao/browser/agent/dao_agent_settings_handler_unittest.cc`:
- Around line 99-121: 在 DaoAgentSettingsHandlerTest 中新增整型计数回归测试：使用
ScopedDictPrefUpdate 向 prefs::kDaoAgentUsageStats 写入完整 schema 的整数值，然后调用
RecordDaoAgentApiUsage 和 RecordDaoAgentToolUsage，断言 BuildDaoAgentUsageStats
返回对应的累加结果，覆盖整数类型不会触发 optional 解引用。
In `@src/dao/browser/agent/dao_agent_settings_handler.cc`:
- Around line 642-687: 在 HandleGetWorkspaceSummary 的 GetUsageInfo 回调中复用发起请求时已获取的
DaoAgentWorkspaceService，避免再次调用 Profile::FromWebUI 和
DaoAgentWorkspaceServiceFactory::GetForProfile。将该 service
传入后续回调，保持服务不存在时的现有空结果处理，并简化嵌套逻辑。
In `@src/dao/browser/agent/dao_agent_settings_handler.h`:
- Around line 21-29: Rename the top-level integer constant
kDaoAgentSettingsMigrationVersion to a schema-specific name such as
kDaoAgentSettingsSchemaVersion, and update every reference to it. Leave the
string pref key constant in dao::prefs unchanged to avoid namespace and lookup
ambiguity.
In `@src/dao/browser/ui/dao_ui_sources.gni`:
- Around line 8-9: 按字典序调整两个 GN 列表中的新增条目：在 src/dao/browser/ui/dao_ui_sources.gni
第 8-9 行，将 dao_agent_settings_handler.cc 和 dao_agent_settings_handler.h 移到
dao_agent_scenario_registry.h 之后、dao_agent_skill_service.cc 之前；在
src/dao/browser/agent/BUILD.gn 第 13 行，将 dao_agent_settings_handler_unittest.cc
移到 dao_agent_scenario_registry_unittest.cc
之后、dao_agent_workspace_service_unittest.cc 之前，确保列表保持有序并避免 gn format 产生额外 diff。
In `@src/patches/chrome/app/settings_strings.grdp.patch`:
- Around line 205-219: 合并重复的英文源文本消息：移除 IDS_SETTINGS_DAO_AGENT_MODEL_TITLE 和
IDS_SETTINGS_DAO_AGENT_MANAGEMENT_TITLE，保留对应的
IDS_SETTINGS_DAO_AGENT_GROUP_MODEL_AND_CONNECTION 与
IDS_SETTINGS_DAO_AGENT_GROUP_DATA_AND_MANAGEMENT 分组标题消息，并更新所有引用以使用保留的消息标识符。
- Around line 193-201: Remove the unreferenced
IDS_SETTINGS_DAO_AGENT_CONFIGURE_TITLE,
IDS_SETTINGS_DAO_AGENT_CONFIGURE_DESCRIPTION, and
IDS_SETTINGS_DAO_AGENT_SUMMARY_UNAVAILABLE messages from the GRDP patch, along
with their registrations in settings_localized_strings_provider.cc.patch and any
corresponding translations. Do not alter other Dao Agent strings or WebUI
behavior.
In
`@src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch`:
- Around line 465-499: Keep formatCost_ fixed to USD for the dollar-denominated
estimate, and update formatTimestamp_ to return the established placeholder text
when value represents an uninitialized timestamp such as 0; otherwise preserve
the existing localized date-time formatting.
In
`@src/patches/chrome/browser/resources/settings/dao_page/dao_agent_settings_browser_proxy.ts.patch`:
- Around line 41-49: 为 DaoAgentUsageStats 增加与 isDaoAgentMemorySummary 和
isDaoAgentWorkspaceSummary 一致的运行时类型守卫 isDaoAgentUsageStats，校验所有字段及 toolCalls
的结构；在 dao_agent_page.ts 的 loadUsageStats_ 与 updateUsageStats_
中先使用该守卫验证原生负载，只有通过校验后才更新或展开 stats.toolCalls，并安全处理无效数据以避免 listener 抛出异常。
🪄 Autofix
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: c9e59a92-7a92-4c69-8551-3d1d1d706552
📒 Files selected for processing (58)
docs/feature-checklist.mddocs/features.mdscripts/commands/__tests__/settings_i18n.test.tsscripts/commands/__tests__/settings_redesign_contract.test.tssrc/dao/browser/agent/BUILD.gnsrc/dao/browser/agent/dao_agent_settings_handler.ccsrc/dao/browser/agent/dao_agent_settings_handler.hsrc/dao/browser/agent/dao_agent_settings_handler_unittest.ccsrc/dao/browser/dao_pref_names.ccsrc/dao/browser/dao_pref_names.hsrc/dao/browser/ui/dao_ui_sources.gnisrc/dao/browser/ui/webui/dao_agent_ui.ccsrc/dao/browser/ui/webui/resources/agent/BUILD.gnsrc/dao/browser/ui/webui/resources/agent/__tests__/agent_bridge_call_native.test.tssrc/dao/browser/ui/webui/resources/agent/__tests__/agent_settings_native_bridge.test.tssrc/dao/browser/ui/webui/resources/agent/__tests__/agent_settings_sync.test.tssrc/dao/browser/ui/webui/resources/agent/__tests__/dao_agent_app.test.tssrc/dao/browser/ui/webui/resources/agent/__tests__/dao_settings_view.test.tssrc/dao/browser/ui/webui/resources/agent/agent.tssrc/dao/browser/ui/webui/resources/agent/agent_bridge.tssrc/dao/browser/ui/webui/resources/agent/agent_settings_native_bridge.tssrc/dao/browser/ui/webui/resources/agent/agent_settings_sync.tssrc/dao/browser/ui/webui/resources/agent/dao_agent_app.tssrc/dao/browser/ui/webui/resources/agent/dao_settings_view.tssrc/patches/chrome/app/resources/generated_resources_zh-CN.xtb.patchsrc/patches/chrome/app/settings_strings.grdp.patchsrc/patches/chrome/browser/resources/settings/BUILD.gn.patchsrc/patches/chrome/browser/resources/settings/a11y_page/a11y_page_index.html.patchsrc/patches/chrome/browser/resources/settings/appearance_page/appearance_page_index.html.patchsrc/patches/chrome/browser/resources/settings/autofill_page/autofill_page_index.html.patchsrc/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.html.patchsrc/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patchsrc/patches/chrome/browser/resources/settings/dao_page/dao_agent_settings_browser_proxy.ts.patchsrc/patches/chrome/browser/resources/settings/dao_page/dao_page.ts.patchsrc/patches/chrome/browser/resources/settings/lazy_load.ts.patchsrc/patches/chrome/browser/resources/settings/people_page/people_page_index.html.patchsrc/patches/chrome/browser/resources/settings/privacy_page/privacy_page_index.ts.patchsrc/patches/chrome/browser/resources/settings/route.ts.patchsrc/patches/chrome/browser/resources/settings/router_dao.ts.patchsrc/patches/chrome/browser/resources/settings/settings.ts.patchsrc/patches/chrome/browser/resources/settings/settings_main/settings_main.html.patchsrc/patches/chrome/browser/resources/settings/settings_main/settings_main.ts.patchsrc/patches/chrome/browser/resources/settings/settings_menu/settings_menu.html.patchsrc/patches/chrome/browser/resources/settings/settings_menu/settings_menu.ts.patchsrc/patches/chrome/browser/resources/settings/settings_page/settings_section.html.patchsrc/patches/chrome/browser/resources/settings/settings_shared.css.patchsrc/patches/chrome/browser/resources/settings/settings_ui/settings_ui.html.patchsrc/patches/chrome/browser/resources/settings/settings_ui/settings_ui.ts.patchsrc/patches/chrome/browser/resources/settings/your_saved_info_page/your_saved_info_page_index.html.patchsrc/patches/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc.patchsrc/patches/chrome/browser/ui/webui/settings/settings_ui.cc.patchsrc/patches/chrome/test/data/webui/settings/BUILD.gn.patchsrc/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patchsrc/patches/chrome/test/data/webui/settings/dao_page_test.ts.patchsrc/patches/chrome/test/data/webui/settings/settings_browsertest.cc.patchsrc/patches/chrome/test/data/webui/settings/settings_main_test.ts.patchsrc/patches/chrome/test/data/webui/settings/settings_menu_test.ts.patchsrc/patches/chrome/test/data/webui/settings/settings_ui_test.ts.patch
💤 Files with no reviewable changes (2)
- src/dao/browser/ui/webui/resources/agent/dao_settings_view.ts
- src/dao/browser/ui/webui/resources/agent/tests/dao_settings_view.test.ts
| |---|---------|--------------------|------|----------------------| | ||
| | ☐ | 4 keyed-service factories registered (memory, skill, workspace, dream) | `profiles/chrome_browser_main_extra_parts_profiles.cc.patch` | 🟡 | Services instantiate per profile; agent features work | | ||
| | ☐ | Agent WebUI host allowed to make network requests (LLM API) | `webui/chrome_web_ui_controller_factory.cc.patch` (`origin.host()=="agent"`) | 🟢 | `dao://agent` reaches external LLM endpoints | | ||
| | ☐ | Unified Profile-scoped Agent settings and legacy migration | `src/dao/.../agent/dao_agent_settings_handler.{h,cc}`, `resources/agent/agent_settings_{sync,native_bridge}.ts`, `resources/settings/dao_page/dao_page.{html,ts}.patch`, `webui/settings/settings_ui.cc.patch` | 🟡 | Run `agent_settings_sync.test.ts`, `dao_agent_app.test.ts`, `DaoPage`, and `DaoAgentSettingsHandlerTest`; verify legacy `dao://agent` local-storage values migrate once without overwriting Settings values, partial usage dictionaries receive validated defaults and derived totals, malformed/non-finite fields fail closed, canonical stored snapshots still require the complete schema, both WebUIs receive `dao-agent-settings-changed`, rapid tool toggles serialize cumulative disabled-tool arrays and resync after failure, the resume window defaults to 3 and accepts 0, runtime-only state is not migrated, and the Agent gear opens `dao://settings/#dao` without rendering a duplicate settings view | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
修正 Agent 设置入口的锚点。
本行要求验证 “the Agent gear opens dao://settings/#dao”。但 docs/features.md 第 151 行与第 175 行、以及 scripts/commands/__tests__/settings_redesign_contract.test.ts 第 567 行都以 dao://settings/#agent 为准。Agent 设置已成为与 dao 并列的独立顶层区块，锚点应为 #agent。按当前文本执行验证会跳转到错误区块。
📝 建议修改
-runtime-only state is not migrated, and the Agent gear opens `dao://settings/#dao` without rendering a duplicate settings view |
+runtime-only state is not migrated, and the Agent gear opens `dao://settings/#agent` without rendering a duplicate settings view |📝 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.
Suggested change
| | ☐ | Unified Profile-scoped Agent settings and legacy migration | `src/dao/.../agent/dao_agent_settings_handler.{h,cc}`, `resources/agent/agent_settings_{sync,native_bridge}.ts`, `resources/settings/dao_page/dao_page.{html,ts}.patch`, `webui/settings/settings_ui.cc.patch` | 🟡 | Run `agent_settings_sync.test.ts`, `dao_agent_app.test.ts`, `DaoPage`, and `DaoAgentSettingsHandlerTest`; verify legacy `dao://agent` local-storage values migrate once without overwriting Settings values, partial usage dictionaries receive validated defaults and derived totals, malformed/non-finite fields fail closed, canonical stored snapshots still require the complete schema, both WebUIs receive `dao-agent-settings-changed`, rapid tool toggles serialize cumulative disabled-tool arrays and resync after failure, the resume window defaults to 3 and accepts 0, runtime-only state is not migrated, and the Agent gear opens `dao://settings/#dao` without rendering a duplicate settings view | | |
| | ☐ | Unified Profile-scoped Agent settings and legacy migration | `src/dao/.../agent/dao_agent_settings_handler.{h,cc}`, `resources/agent/agent_settings_{sync,native_bridge}.ts`, `resources/settings/dao_page/dao_page.{html,ts}.patch`, `webui/settings/settings_ui.cc.patch` | 🟡 | Run `agent_settings_sync.test.ts`, `dao_agent_app.test.ts`, `DaoPage`, and `DaoAgentSettingsHandlerTest`; verify legacy `dao://agent` local-storage values migrate once without overwriting Settings values, partial usage dictionaries receive validated defaults and derived totals, malformed/non-finite fields fail closed, canonical stored snapshots still require the complete schema, both WebUIs receive `dao-agent-settings-changed`, rapid tool toggles serialize cumulative disabled-tool arrays and resync after failure, the resume window defaults to 3 and accepts 0, runtime-only state is not migrated, and the Agent gear opens `dao://settings/#agent` without rendering a duplicate settings view | |
🤖 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 `@docs/feature-checklist.md` at line 105, Update the checklist entry’s Agent
gear verification requirement to use the canonical `dao://settings/#agent`
anchor instead of `dao://settings/#dao`, keeping it consistent with the
documented settings contract and `settings_redesign_contract.test.ts`.
| expect(agentHtml).not.toContain("<settings-subpage"); | ||
| expect(agentTs).toContain("implements SettingsPlugin"); | ||
| expect(agentTs).toContain("searchContents(query: string)"); | ||
| + expect(agentTs).toContain("getSearchManager().search(query, this)"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
删除残留的 diff 标记 +。
本行以 + 开头，属于补丁残留。它会被解析为一元加表达式，断言仍会执行，但该记号是无效产物，且会被 lint 拒绝。
🐛 建议修复
-+ expect(agentTs).toContain("getSearchManager().search(query, this)");
+ expect(agentTs).toContain("getSearchManager().search(query, this)");📝 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.
Suggested change
| + expect(agentTs).toContain("getSearchManager().search(query, this)"); | |
| expect(agentTs).toContain("getSearchManager().search(query, this)"); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@scripts/commands/__tests__/settings_redesign_contract.test.ts` at line 421,
Remove the leading “+” diff marker from the assertion in
settings_redesign_contract.test.ts, leaving the expect call as a normal test
statement so it passes lint.
Comment on lines
+252
to
+257
| void PrepareUsageStatsForUpdate(ScopedDictPrefUpdate* update) { | ||
| base::DictValue normalized; | ||
| if (!NormalizeUsageStats(update->Get(), &normalized)) { | ||
| StoreUsageStats(update, NewUsageStats(base::Time::Now())); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
PrepareUsageStatsForUpdate 不回写规范化结果，会导致下游解引用空 optional。
NormalizeUsageStats 通过 ReadNonNegativeFiniteNumber 接受整数值（value->is_int()），但校验通过时该函数把原始字典原样保留。如果 dao.agent_usage_stats 中的某个计数以整型存储（例如被外部工具改写或历史数据），后续代码会崩溃：
- Line 368-376
RecordDaoAgentApiUsage：*update->FindDouble(kUsageStatsApiCalls)在值为整型时FindDouble返回std::nullopt，解引用即未定义行为。 - Line 401-402
RecordDaoAgentToolUsage：*existing_value->GetIfDouble()同理。
请在校验成功时写回规范化后的字典，使所有数值统一为 double。
🐛 建议的修复
void PrepareUsageStatsForUpdate(ScopedDictPrefUpdate* update) {
base::DictValue normalized;
- if (!NormalizeUsageStats(update->Get(), &normalized)) {
- StoreUsageStats(update, NewUsageStats(base::Time::Now()));
- }
+ if (NormalizeUsageStats(update->Get(), &normalized)) {
+ StoreUsageStats(update, std::move(normalized));
+ return;
+ }
+ StoreUsageStats(update, NewUsageStats(base::Time::Now()));
}📝 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.
Suggested change
| void PrepareUsageStatsForUpdate(ScopedDictPrefUpdate* update) { | |
| base::DictValue normalized; | |
| if (!NormalizeUsageStats(update->Get(), &normalized)) { | |
| StoreUsageStats(update, NewUsageStats(base::Time::Now())); | |
| } | |
| } | |
| void PrepareUsageStatsForUpdate(ScopedDictPrefUpdate* update) { | |
| base::DictValue normalized; | |
| if (NormalizeUsageStats(update->Get(), &normalized)) { | |
| StoreUsageStats(update, std::move(normalized)); | |
| return; | |
| } | |
| StoreUsageStats(update, NewUsageStats(base::Time::Now())); | |
| } |
🤖 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/dao/browser/agent/dao_agent_settings_handler.cc` around lines 252 - 257,
Update PrepareUsageStatsForUpdate to write the normalized dictionary back into
update when NormalizeUsageStats succeeds, ensuring integer counters are
converted to doubles before downstream consumers such as RecordDaoAgentApiUsage
and RecordDaoAgentToolUsage access them. Preserve the existing NewUsageStats
fallback when normalization fails.
Comment on lines
+461
to
+479
| if (prefs->GetInteger(prefs::kDaoAgentSettingsMigrationVersion) < 1) { | ||
| size_t total_bytes = 0; | ||
| { | ||
| ScopedDictPrefUpdate update(prefs, prefs::kDaoAgentSettings); | ||
| for (const auto [key, value] : legacy_values) { | ||
| if (!IsManagedDaoAgentSetting(key) || !value.is_string() || | ||
| key == kDaoAgentMemoryEnabledSetting || | ||
| key == kDaoDreamEnabledSetting || key == kDaoDreamDebugSetting || | ||
| key == kDaoDreamExcludedDomainsSetting || update->contains(key)) { | ||
| continue; | ||
| } | ||
| total_bytes += value.GetString().size(); | ||
| if (value.GetString().size() > kMaxSettingValueBytes || | ||
| total_bytes > kMaxSettingsBytes) { | ||
| continue; | ||
| } | ||
| update->Set(key, value.GetString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
超限值也会计入 total_bytes，可能阻断后续合法值的迁移。
Line 472 在做大小检查之前先累加 total_bytes。当遇到一个超过 kMaxSettingValueBytes 的值时，该值被跳过，但它的字节数仍然计入总量，可能使后面的合法值触发 total_bytes > kMaxSettingsBytes 而被丢弃。请先检查单值上限，再累加。
🐛 建议的修复
- total_bytes += value.GetString().size();
- if (value.GetString().size() > kMaxSettingValueBytes ||
- total_bytes > kMaxSettingsBytes) {
+ if (value.GetString().size() > kMaxSettingValueBytes ||
+ total_bytes + value.GetString().size() > kMaxSettingsBytes) {
continue;
}
+ total_bytes += value.GetString().size();📝 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.
Suggested change
| if (prefs->GetInteger(prefs::kDaoAgentSettingsMigrationVersion) < 1) { | |
| size_t total_bytes = 0; | |
| { | |
| ScopedDictPrefUpdate update(prefs, prefs::kDaoAgentSettings); | |
| for (const auto [key, value] : legacy_values) { | |
| if (!IsManagedDaoAgentSetting(key) || !value.is_string() || | |
| key == kDaoAgentMemoryEnabledSetting || | |
| key == kDaoDreamEnabledSetting || key == kDaoDreamDebugSetting || | |
| key == kDaoDreamExcludedDomainsSetting || update->contains(key)) { | |
| continue; | |
| } | |
| total_bytes += value.GetString().size(); | |
| if (value.GetString().size() > kMaxSettingValueBytes || | |
| total_bytes > kMaxSettingsBytes) { | |
| continue; | |
| } | |
| update->Set(key, value.GetString()); | |
| } | |
| } | |
| if (prefs->GetInteger(prefs::kDaoAgentSettingsMigrationVersion) < 1) { | |
| size_t total_bytes = 0; | |
| { | |
| ScopedDictPrefUpdate update(prefs, prefs::kDaoAgentSettings); | |
| for (const auto [key, value] : legacy_values) { | |
| if (!IsManagedDaoAgentSetting(key) || !value.is_string() || | |
| key == kDaoAgentMemoryEnabledSetting || | |
| key == kDaoDreamEnabledSetting || key == kDaoDreamDebugSetting || | |
| key == kDaoDreamExcludedDomainsSetting || update->contains(key)) { | |
| continue; | |
| } | |
| if (value.GetString().size() > kMaxSettingValueBytes || | |
| total_bytes + value.GetString().size() > kMaxSettingsBytes) { | |
| continue; | |
| } | |
| total_bytes += value.GetString().size(); | |
| update->Set(key, value.GetString()); | |
| } | |
| } |
🤖 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/dao/browser/agent/dao_agent_settings_handler.cc` around lines 461 - 479,
Update the migration loop in the DaoAgentSettings migration block so each value
is checked against kMaxSettingValueBytes before adding its size to total_bytes;
only valid-sized values should contribute to the aggregate limit and proceed to
update->Set.
Comment on lines
+563
to
+590
| void DaoAgentSettingsHandler::HandleGetSettings(const base::ListValue& args) { | ||
| AllowJavascript(); | ||
| if (args.size() != 1 || !args[0].is_string() || !GetPrefs()) { | ||
| return; | ||
| } | ||
| ResolveJavascriptCallback(args[0], BuildDaoAgentSettingsSnapshot(GetPrefs())); | ||
| } | ||
|
|
||
| void DaoAgentSettingsHandler::HandleMigrateLegacySettings( | ||
| const base::ListValue& args) { | ||
| AllowJavascript(); | ||
| if (args.size() != 2 || !args[0].is_string() || !args[1].is_dict() || | ||
| !GetPrefs()) { | ||
| return; | ||
| } | ||
| ResolveJavascriptCallback( | ||
| args[0], MigrateLegacyDaoAgentSettings(GetPrefs(), args[1].GetDict())); | ||
| } | ||
|
|
||
| void DaoAgentSettingsHandler::HandleSetSetting(const base::ListValue& args) { | ||
| AllowJavascript(); | ||
| if (args.size() != 3 || !args[0].is_string() || !args[1].is_string() || | ||
| !GetPrefs()) { | ||
| return; | ||
| } | ||
| ResolveJavascriptCallback( | ||
| args[0], SetDaoAgentSetting(GetPrefs(), args[1].GetString(), args[2])); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
参数校验失败时不回调，前端 Promise 会永久挂起。
HandleGetSettings、HandleMigrateLegacySettings、HandleSetSetting 在参数非法或 GetPrefs() 为空时直接 return，不调用 ResolveJavascriptCallback 或 RejectJavascriptCallback。sendWithPromise 的调用方会一直等待，设置页可能停在加载态。HandleResetUsageStats（Line 714-726）已经处理了这种情况，建议其余处理函数保持一致，至少调用 RejectJavascriptCallback。
🤖 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/dao/browser/agent/dao_agent_settings_handler.cc` around lines 563 - 590,
Update HandleGetSettings, HandleMigrateLegacySettings, and HandleSetSetting so
every validation-failure or missing-GetPrefs path invokes
RejectJavascriptCallback with the supplied callback identifier before returning.
Match the existing failure-handling pattern in HandleResetUsageStats, while
preserving the current successful ResolveJavascriptCallback behavior.
| + declare prefs: {[key: string]: any}; | ||
| + private agentSettingsBrowserProxy_: DaoAgentSettingsBrowserProxy = | ||
| + DaoAgentSettingsBrowserProxyImpl.getInstance(); | ||
| + private agentSettingsValues_: Record<string, string> = {}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
工具开关不会随快照重新渲染。 根因是 agentSettingsValues_ 作为模板绑定依赖，却以普通类字段声明。类字段初始化器会在实例上覆盖 Polymer 创建的原型访问器，后续重新赋值不再通知模板。
src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch#L124-L124：把agentSettingsValues_加入static get properties()，并改用declare private agentSettingsValues_: Record<string, string>;，去掉字段初始化器。src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.html.patch#L639-L648：声明修正后保留现有绑定，并补充一条断言快照更新后单个工具开关状态同步的测试。
📍 Affects 2 files
src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch#L124-L124(this comment)src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.html.patch#L639-L648
🤖 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/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch`
at line 124, Update agentSettingsValues_ in the Polymer element by declaring it
in static get properties() and replacing the initialized class field with
declare private agentSettingsValues_: Record<string, string>; so the generated
reactive accessor is preserved. In
src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.ts.patch
lines 124-124, apply the declaration change; in
src/patches/chrome/browser/resources/settings/dao_page/dao_agent_page.html.patch
lines 639-648, retain the existing binding and add an assertion verifying an
individual tool toggle synchronizes after snapshot rerender.
Comment on lines
+159
to
+162
| + this.setupOverviewObserver_(); | ||
| + if (!this.inSearchMode_) { | ||
| + this.restoreOverviewScroll(); | ||
| + } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
清除搜索后恢复滚动位置时可能读到过期的 scrollHeight。
restoreOverviewScroll 在搜索结果应用后的同一个任务中同步调用。此时被 hidden-by-search 隐藏的分区刚刚恢复显示，布局尚未更新，container.scrollHeight 仍是搜索状态下的值。maxScroll 因此偏小，恢复位置被截断。
请把恢复动作推迟到下一次渲染之后。
🐛 建议修复
if (!this.inSearchMode_) {
- this.restoreOverviewScroll();
+ requestAnimationFrame(() => this.restoreOverviewScroll());
}Also applies to: 248-251
🤖 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/patches/chrome/browser/resources/settings/settings_main/settings_main.ts.patch`
around lines 159 - 162, Defer the restoreOverviewScroll call in the
search-clearing flow until after the next render, rather than invoking it
synchronously after setupOverviewObserver_. Keep the !this.inSearchMode_ guard
and ensure restoreOverviewScroll reads scrollHeight only after hidden sections
have been laid out.
Comment on lines
+212
to
+220
| + <aside id="daoSettingsNavigation"> | ||
| + <cr-toolbar-search-field id="searchField" | ||
| + label="$i18n{searchPrompt}" | ||
| + clear-label="$i18n{clearSearch}" | ||
| + autofocus | ||
| + spinner-active="[[toolbarSpinnerActive_]]" | ||
| + on-search-changed="onSearchChanged_" | ||
| + showing-search> | ||
| + </cr-toolbar-search-field> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
恢复顶部横幅地标。
已删除的 cr-toolbar 定义了 role="banner"。新的结构只提供 aside 和 main 地标。请使用 header 和 role="banner" 包装顶部搜索区域，以保留屏幕阅读器的页面导航入口。
建议修改
- <aside id="daoSettingsNavigation">
+ <header id="daoSettingsNavigation" role="banner">
<cr-toolbar-search-field id="searchField"
label="$i18n{searchPrompt}"
clear-label="$i18n{clearSearch}"
autofocus
spinner-active="[[toolbarSpinnerActive_]]"
on-search-changed="onSearchChanged_"
showing-search>
</cr-toolbar-search-field>
<settings-menu id="leftMenu"
on-settings-section-activate="onSettingsSectionActivate_">
</settings-menu>
- </aside>
+ </header>🤖 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/patches/chrome/browser/resources/settings/settings_ui/settings_ui.html.patch`
around lines 212 - 220, Wrap the top search area in a header element with
role="banner" around the daoSettingsNavigation aside, preserving the existing
cr-toolbar-search-field configuration and navigation structure so screen readers
retain the banner landmark.
Comment on lines
+181
to
+198
| + private async onSettingsSectionActivate_( | ||
| + event: CustomEvent<{section: string, behavior: ScrollBehavior}>) { | ||
| + const {section, behavior} = event.detail; | ||
| + const router = Router.getInstance(); | ||
| + const hasSearch = !!router.getQueryParameters().get('search'); | ||
| + if (!this.isOverviewRoute_(router.getCurrentRoute()) || hasSearch) { | ||
| + router.navigateTo( | ||
| + routes.BASIC, /* dynamicParams */ undefined, | ||
| + /* removeSearch */ true); | ||
| + await Promise.resolve(); | ||
| + await this.$.main.whenViewSwitchingDone(); | ||
| + await this.searchPromise_; | ||
| + } | ||
| + | ||
| + if (this.$.main.scrollToOverviewSection(section, behavior)) { | ||
| + this.replaceOverviewHash_(section); | ||
| + this.$.leftMenu.setActiveSection(section); | ||
| + } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
在分区激活时取消未提交的搜索。
如果用户在 150 ms 防抖期间选择分区，hasSearch 仍为 false。此方法会滚动到该分区，但待处理的计时器随后调用 commitSearch_，并重新应用旧查询。用户会立即离开刚选择的分区。
在滚动前取消 searchDebounceTimer_。同时清除或提交搜索字段的待处理值，使搜索字段、URL 和概览内容保持一致。请添加“输入搜索后立即选择分区”的回归测试。
建议修改
private async onSettingsSectionActivate_(
event: CustomEvent<{section: string, behavior: ScrollBehavior}>) {
+ if (this.searchDebounceTimer_ !== null) {
+ clearTimeout(this.searchDebounceTimer_);
+ this.searchDebounceTimer_ = null;
+ this.$.searchField.setValue('', true /* noEvent */);
+ }
+
const {section, behavior} = event.detail;📝 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.
Suggested change
| + private async onSettingsSectionActivate_( | |
| + event: CustomEvent<{section: string, behavior: ScrollBehavior}>) { | |
| + const {section, behavior} = event.detail; | |
| + const router = Router.getInstance(); | |
| + const hasSearch = !!router.getQueryParameters().get('search'); | |
| + if (!this.isOverviewRoute_(router.getCurrentRoute()) || hasSearch) { | |
| + router.navigateTo( | |
| + routes.BASIC, /* dynamicParams */ undefined, | |
| + /* removeSearch */ true); | |
| + await Promise.resolve(); | |
| + await this.$.main.whenViewSwitchingDone(); | |
| + await this.searchPromise_; | |
| + } | |
| + | |
| + if (this.$.main.scrollToOverviewSection(section, behavior)) { | |
| + this.replaceOverviewHash_(section); | |
| + this.$.leftMenu.setActiveSection(section); | |
| + } | |
| private async onSettingsSectionActivate_( | |
| event: CustomEvent<{section: string, behavior: ScrollBehavior}>) { | |
| if (this.searchDebounceTimer_ !== null) { | |
| clearTimeout(this.searchDebounceTimer_); | |
| this.searchDebounceTimer_ = null; | |
| this.$.searchField.setValue('', true /* noEvent */); | |
| } | |
| const {section, behavior} = event.detail; | |
| const router = Router.getInstance(); | |
| const hasSearch = !!router.getQueryParameters().get('search'); | |
| if (!this.isOverviewRoute_(router.getCurrentRoute()) || hasSearch) { | |
| router.navigateTo( | |
| routes.BASIC, /* dynamicParams */ undefined, | |
| /* removeSearch */ true); | |
| await Promise.resolve(); | |
| await this.$.main.whenViewSwitchingDone(); | |
| await this.searchPromise_; | |
| } | |
| if (this.$.main.scrollToOverviewSection(section, behavior)) { | |
| this.replaceOverviewHash_(section); | |
| this.$.leftMenu.setActiveSection(section); | |
| } |
🤖 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/patches/chrome/browser/resources/settings/settings_ui/settings_ui.ts.patch`
around lines 181 - 198, Update onSettingsSectionActivate_ to cancel
searchDebounceTimer_ before scrolling or navigating, and clear or commit the
pending search input so the field, URL, and overview remain consistent. Add a
regression test covering selecting a section immediately after entering search
text during the debounce window.
Comment on lines
+530
to
+532
| + assertEquals( | ||
| + loadTimeData.getString('daoAgentManagementMemoryRefreshError'), | ||
| + refreshError.textContent.trim()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the prevailing pattern for textContent access in Chromium settings WebUI tests.
rg -n 'textContent!?\.trim\(\)' --glob '*.patch' --glob '*.ts' | head -40Repository: msgbyte/dao-browser
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching target =="
git ls-files | grep -E '(^src/patches/chrome/test/data/webui/settings/dao_agent_page_test\.ts\.patch$|src/dao/|.*dao.*agent.*test.*\.ts$|.*settings.*test.*\.ts$)' | head -200
echo
echo "== target patch context around reported lines =="
if [ -f src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch ]; then
nl -ba src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch | sed -n '500,670p'
else
echo "missing target patch"
fi
echo
echo "== TypeScript/config presence for strictNullChecks =="
git ls-files | grep -E '(^tsconfig.*\.json$|package\.json$|src/patches/chrome/test/data/webui/settings/dao_agent_page_test\.ts\.tsproj$)' | while read -r f; do
echo "--- $f"
sed -n '1,220p' "$f" | grep -Ei 'strictNullChecks|strict|noImplicitAny|files|include|typeRoots|types|lib|compilerOptions' || true
done
echo
echo "== related textContent trim usage in repo patch/ts files =="
rg -n 'textContent\s*(;\s*trim|!)?\.trim\(\)' --glob '*.patch' --glob '*.ts' | sed -n '1,120p' || true
echo
echo "== HTMLElement references in target patch =="
rg -n 'refreshError|actionError|getAttributeNode|textContent' src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch || trueRepository: msgbyte/dao-browser
Length of output: 10680
🏁 Script executed:
#!/bin/bash
set -ueo pipefail
echo "== target patch existence =="
git status --short
git ls-files | grep -F 'src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch' || true
git ls-files | grep -F 'src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts' || true
echo
echo "== exact target patch context if present =="
if git ls-files | grep -Fq 'src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch'; then
nl -ba src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch | sed -n '500,670p'
else
echo "target patch not tracked"
fi
echo
echo "== Node textContent type evidence from installed types if available =="
node - <<'JS'
const fs = require('fs');
for (const p of [
'node_modules/typescript/lib/lib.dom.d.ts',
'third_party/blink/renderer/bindings/core/templates/lib.dom.d.ts'
]) {
if (!fs.existsSync(p)) continue;
const m = fs.readFileSync(p, 'utf8').match(/(^|\n)\s*textContent[ \t]*:[ \t]*[^\n;\s]+[;\n]/m);
console.log(`-- ${p} --`);
console.log(m ? m[0].split('\n').join('\n') : 'textContent declaration not found');
}
JS
echo
echo "== related textContent/null trim usage in tracked .patch/.ts files =="
rg -n 'textContent\s*(;\s*trim|!)?\.trim\(\)' --glob '*.patch' --glob '*.ts' | sed -n '1,160p' || trueRepository: msgbyte/dao-browser
Length of output: 626
🌐 Web query:
DOM Node textContent TypeScript lib.dom.d.ts declaration string | null trim
💡 Result:
In TypeScript's lib.dom.d.ts, the Node.textContent property is defined as string | null [1]. The null value occurs in specific cases, such as when accessing textContent on a Document or Doctype node [1]. Because the property is typed as string | null, attempting to call the.trim method directly on it (e.g., node.textContent.trim) will trigger a TypeScript compiler error: "Object is possibly 'null'" [2][3][4]. To safely trim the content in TypeScript, you must ensure the value is a string before calling.trim [5][4]. You can handle this using optional chaining, type guards, or nullish coalescing [6][7][5]. Recommended approaches: 1. Optional Chaining: Use the?. operator to safely call trim only if the value is not null or undefined. If the value is null, the expression will return undefined [7][5]. const trimmed = node.textContent?.trim; 2. Nullish Coalescing: Provide a fallback string (e.g., an empty string) if the value is null [6]. const trimmed = (node.textContent?? "").trim; 3. Type Guard: Explicitly check that the value is a string [4]. const content = node.textContent; if (typeof content === "string") { const trimmed = content.trim; } While there have been community discussions and efforts to refine these type definitions to be more precise for specific element types (where textContent is never null), the primary Node interface definition remains string | null for broad compatibility with the DOM specification [8][2][9].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent
- 2: 'innerText' should not be nullable in most of Node's derived types microsoft/TypeScript#10315
- 3: 'innerText' should not be nullable in most of Node's derived types microsoft/TypeScript#10315
- 4: https://stackoverflow.com/questions/61125262/writing-sanitizer-function-in-typescript-to-check-for-null-undefined-and-empty
- 5: https://www.xjavascript.com/blog/typescript-trim/
- 6: https://stackoverflow.com/questions/54767083/trim-string-without-error-even-when-its-null-or-undefined
- 7: "Cannot read properties of null" when using trim with nullish open-circle/valibot#770
- 8: Make most textContent not nullable microsoft/TypeScript-DOM-lib-generator#1942
- 9: Element.textContent should not be nullable microsoft/TypeScript-DOM-lib-generator#1939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f='src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch'
echo "== target chunk context with surrounding tests =="
nl -ba "$f" | sed -n '460,675p'
echo
echo "== refreshError/actionError declarations and DOM access context =="
nl -ba "$f" | sed -n '420,570p' | grep -nE 'querySelector|getElementById|getText|querySelectorAll|textContent' -C 5 || true
echo
echo "== DOM element query patterns nearby =="
python3 - <<'PY'
from pathlib import Path
p=Path('src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch')
lines=p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 'querySelector' in l or 'getElementById' in l or l.strip().startswith('+'):
if any(x in ''.join(lines[max(1,i-20):i+20]) for x in ['refreshError','actionError','querySelector']):
print(f'\n--- block around line {i} ---')
for j in range(max(1,i-25), min(len(lines),i+30)+1):
print(f'{j:04}: {lines[j-1]}')
PYRepository: msgbyte/dao-browser
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f='src/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch'
echo "== target chunk context with surrounding tests =="
awk '{printf "%5d\t%s\n", NR, $0}' "$f" | sed -n '460,675p'
echo
echo "== refreshError/actionError declarations and DOM access context =="
awk '{printf "%5d\t%s\n", NR, $0}' "$f" | sed -n '420,675p' | grep -nE 'querySelector|getElementById|getText|querySelectorAll|textContent|refreshError|actionError' -C 6 || trueRepository: msgbyte/dao-browser
Length of output: 20178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig and compiler strict settings for WebUI TypeScript files =="
rg -n 'strictNullChecks|allowJs|target|lib|include|include' --glob 'tsconfig*.json' --glob '*.gn' --glob '*.jsonc' --glob '*.json' . | sed -n '1,240p'
echo
echo "== BUILD.gn/type compiler hints under webui settings chromium/test =="
git ls-files | grep -E '(^src/chrome/test/data/webui/settings/.*BUILD\.gn$|(^|/)BUILD\.gn$)' | grep -E 'chrome/test/data/webui/settings|webui/settings' | head -80 | while read -r f; do
echo "--- $f"
grep -En 'typescript|compiler|strictNullChecks|tsconfig|lib|src:.*dao_agent_page_test|patch|chromium' "$f" || true
done
echo
echo "== relevant source component around error message element ids =="
rg -n 'memoryRefreshErrorMessage|usageRefreshErrorMessage|memoryActionError|usageActionError|loadTimeData\.getString\(.*daoAgentManagement.*(Refresh|Action).*Error' --glob '*.ts' --glob '*.html' --glob '*.tsx' --glob '*.patch' . | sed -n '1,220p'Repository: msgbyte/dao-browser
Length of output: 26588
🌐 Web query:
Chromium ts_library strictNullChecks lib.dom.d.ts lib.es6 typescript chromium
💡 Result:
In the Chromium codebase, ts_library is a custom build rule used to manage TypeScript compilation, which enforces strict control over the build environment to ensure consistency across the project [1][2][3]. Regarding the specific configuration points: StrictNullChecks: The Chromium build infrastructure typically manages TypeScript compiler options via shared base configurations (e.g., tsconfig_base.json) [4][5]. During the migration of Chromium components from JavaScript to TypeScript, base configurations were often initially set to disable strict checks to facilitate the transition [4]. Over time, these configurations are managed to ensure type safety, and individual ts_library targets are constrained by validation scripts that prevent unsupported compiler options from being overridden manually [3]. lib.dom.d.ts: The inclusion of standard library definitions like lib.dom.d.ts is generally handled through the TypeScript compiler's default behavior or explicitly defined within the tsconfig inheritance chain used by ts_library [1][5]. Chromium's ts_library implementation is designed to work with these standard definitions while specifically managing custom type dependencies (e.g., trusted-types) [5]. ts_library Infrastructure: - Validation: Chromium uses internal tools like validate_tsconfig.py to ensure that tsconfig files do not contain unsupported compiler options [3]. Many options must be set via specific GN arguments passed to ts_library rather than directly in the tsconfig file [3]. - Dependencies: ts_library handles dependencies on other TypeScript libraries, ensuring that declaration files (.d.ts) are correctly resolved and that the build process remains incremental [2][6][7]. - Optimization: To speed up builds, Chromium's ts_library implementation automatically sets skipLibCheck to true in certain scenarios (e.g., when no local definitions exist outside of tools/typescript/definitions), unless it has been explicitly specified in the parent configuration [1][5]. For developers working within Chromium, it is standard practice to rely on the established tsconfig hierarchy rather than attempting to override these core settings at the individual target level [3].
Citations:
- 1: https://chromium.googlesource.com/chromium/src/+/refs/tags/140.0.7281.1/tools/typescript/ts_library.py
- 2: https://chromium.googlesource.com/chromium/src/+/110.0.5481.77/tools/typescript/ts_library.py
- 3: https://chromium.googlesource.com/chromium/src/+/refs/tags/135.0.7049.116/tools/typescript/validate_tsconfig.py
- 4: chromium/chromium@b404309
- 5: https://chromium.googlesource.com/chromium/src.git/+/fb6bbc8482a26e5fb4e822e78f574695484c5ab8/tools/typescript/ts_library.py
- 6: https://chromium.googlesource.com/chromium/src/+/refs/tags/135.0.6999.1/tools/typescript/ts_library.py
- 7: https://github.com/angular/angular-bazel-example/wiki/TypeScript-rules
为 textContent.trim() 添加显式非空处理。
refreshError 与 actionError 的类型是 HTMLElement，Node.textContent 类型是 string|null。加上 .trim() 可能在开启 strictNullChecks 的 Chromium ts_library 测试中触发编译错误。四处相同调用行分别是 532、555、629、652。
🐛 建议修复
- refreshError.textContent.trim());
+ refreshError.textContent!.trim());- actionError.textContent.trim());
+ actionError.textContent!.trim());🤖 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/patches/chrome/test/data/webui/settings/dao_agent_page_test.ts.patch`
around lines 530 - 532, Update the four assertions using refreshError or
actionError textContent at the referenced locations to explicitly handle the
nullable textContent value before trimming, while preserving the existing string
comparison behavior and expected error messages.
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Unify Dao settings into a continuous settings overview and move Agent configuration/management out of the Agent WebUI into the Profile-scoped settings surface.
Changes
Testing
Patch adds/updates WebUI, contract, i18n, and C++ unit test coverage for settings redesign, Agent settings sync, management state, usage persistence, and settings navigation. No test run evidence was provided.
Summary by CodeRabbit
New Features
Bug Fixes
Tests