fix(dashboard): survive unavailable browser storage - #4159
huangruiteng merged 2 commits into
Conversation
Signed-off-by: Green hats <183933879+Green-hats@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
评审绑定 exact head:9e7599b0b5811c89495c24a963b0452384f67c5e。
动机
这个 PR 修复的是一个真实的浏览器能力边界:loadStatusSourceCatalog() 虽然会捕获 getItem 异常,但调用方在进入函数之前就要先求值 window.localStorage。在隐私限制或 sandbox 环境里,如果 property getter 本身抛错,Dashboard 会在初始化阶段失败,连内置的 Local source 都无法显示。
改动思路
改动把异常边界放在已有 owner DashboardPage 的 catalog initializer:只有成功取得 storage 后才调用原 loader,否则复用 emptyStatusSourceCatalog()。现有 persistStatusSourceCatalog 也继续负责写入失败,因而 working-storage 路径完全不变,受限环境则明确降级为当前 session 内可用、reload 后不保留新增 source。
具体改动
dashboard-page.tsx给window.localStorage的取得与 catalog 加载增加同一层保护,不引入第二套状态或 storage abstraction。- 现有 Playwright source-switch smoke 增加 property getter denied 与 storage methods denied 两组真实浏览器场景,验证 Local source 渲染、session 内添加/切换 remote source、reload 后回退,以及没有 uncaught page error。
- 同步重建 packaged Chat 资源。我扫描了 dashboard 中其余 localStorage 调用,它们各自已有异常边界;这次不需要扩成通用 storage framework。
对主干的风险
行为证据是充分的:npm run build、npm run smoke:status-sources、独立端口上的 dev 与 packaged 浏览器 smoke、git diff --check 均通过;同一 head 测试 harness 跑在 base 081ff998f 时,会在 getter-denied 场景等待 Local Goal Only 超时,而本 head 全部通过。
但当前 exact head 还不能进入主干。GitHub 报告 CONFLICTING/DIRTY,本地 git merge-tree --write-tree origin/main HEAD 也只在 loopx/web/chat/index.html 发现冲突。手写 TypeScript source 可以干净合并,冲突来自 stale generated asset hash;手工挑一边可能让 packaged /chat 引用错误 bundle。
我的整体评价
结论是 REQUEST_CHANGES,唯一阻塞项是:rebase 当前 main,从合并后的 source 重新 build packaged Chat 资源,再跑一次 build、dev/packaged status-source browser smoke 与干净 merge-tree。源码修复本身小而正确,真实 failure path 与 session-only fallback 也已经被 mutation-sensitive 地验证;不需要扩大产品范围。
English verdict: REQUEST_CHANGES — exact head 9e7599b0b5811c89495c24a963b0452384f67c5e fixes the pre-call window.localStorage getter failure and passes real dev/packaged browser coverage, but it must rebase current main and regenerate the packaged Chat index/assets because the exact head is currently conflicting.
Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com>
huangruiteng
left a comment
There was a problem hiding this comment.
动机
用户明确要求把 #4159 修复纳入这次 release。当前主干在 window.localStorage 属性 getter 本身抛出 SecurityError 时,会先于 catalog loader 的异常处理在 Dashboard 初始化阶段失败,本机 Goal 连首屏都显示不出来。这个问题在受限 WebView 或隐私环境下会反复发生;修复目标是保住本地状态视图,并诚实呈现无法持久化的边界。
改动思路
沿用当前 DashboardPage 和 status-source catalog 的职责划分:在调用现有 loader 之前把 storage 属性读取包进初始化的异常边界,失败时返回已有的 emptyStatusSourceCatalog();正常存储路径完全交回原 loader。后续添加来源仍由现有持久化函数先更新页面状态,写入失败则只在当前会话可用。合并最新主干时保留了新增的 catalog ref,并删去旧分支的过期 hash 资源;当前发布流程会从源码构建 packaged Chat。
具体改动
确切 head e5eda31940e1d5248ad48c73fee2ea93a104526c 相对主干只改两处:dashboard-page.tsx 的懒初始化增加受限 storage 的降级;status-source-switch-browser-smoke.mjs 增加 getter 与方法拒绝两类真实浏览器用例。测试在每类故障下检查本机 Goal 出现、添加只读 Session Remote、切换回本机、刷新后回到本机,以及没有 uncaught page error;原有来源切换竞态也保留。
关键代码讲解
DashboardPage的statusSourceCataloginitializer 是失败发生的确切边界。原来的调用在求值window.localStorage时就会抛错;现在失败时返回已有 catalog 类型的本机空目录,不发明另一套来源状态。persistStatusSourceCatalog仍是添加来源后的既有路径,先更新 React 状态,再尝试保存。因此被拒绝写入时,用户可以在当前页使用只读来源;下一次刷新不会伪装成已持久保存。正常存储仍按旧规则加载和保存。
复现敏感性也核对过:把新增浏览器用例放到当前主干,同一 getter-denied 场景在等待本机 Goal 时超时;在当前 head,开发版和重新打包的 Chat 浏览器测试都通过。这样验证的是用户真正进入页面的路径,而非只让 mock loader 返回一个空对象。
对主干的风险
最强反例是过宽的 catch 吞掉损坏数据,或让临时来源看起来像持久来源。此次 catch 只位于初始读取边界;原 catalog loader、来源 selector 和写入路径继续负责各自语义。受限环境的来源在刷新后消失已经被浏览器用例明确检查。npm run build、catalog smoke、开发与 packaged 浏览器 smoke 均通过;风险 premerge 执行 8 项,公开边界扫描和差异检查也通过,没有失败、跳过或手工 hold。测试中的远程状态响应是合成数据,因此不把它当成真实远程凭证或写权限的证明。
我的整体评价
旧评审的唯一阻塞是生成资源与主干冲突,如今已通过合并当前主干、采用现行打包流程并复测解决;PR 不再携带过期 bundle。对用户而言,受限存储从白屏变为可继续工作的本机 Dashboard;长期使用则有明确的会话内来源生命周期,而不会误称已经保存。改动集中在现有 owner,范围和风险相称,当前确切 head 没有阻断发现。我批准这个修复进入主干;最终 release 仍需按标签提交另做整版资格核验。
English verdict: APPROVE - exact head e5eda31; current-main getter-denied browser regression fails, while this head passes development and packaged browser checks, build, catalog smoke and 8/8 premerge checks. The stale generated-asset conflict is resolved.
|
Thank you, @Green-hats, for identifying the storage-access failure and contributing a focused fix. I’m sorry I overlooked your PR for so long and left the generated-asset conflict unresolved. I’ve now integrated your change with the current Dashboard, verified the source and packaged browser paths, and merged it. I’ll credit your contribution in the upcoming release notes. |
Problem and result
In browsers where reading
window.localStorageitself throws, Dashboard previously failed before the status-source catalog loader could catch an error. The Dashboard now catches that access at initialization and falls back to the existing in-memory local catalog. A read-only remote source can still be added and used in the current page session; if storage stays unavailable, reloading returns to the local source.The fix stays in
DashboardPageand reusesemptyStatusSourceCatalog. The browser regression covers denied storage getters and methods, local startup, session-only remote source selection and reload, as well as the existing source-switch ordering checks. Current main generates the packaged Chat frontend at build time, so the outdated tracked asset bundle from the original branch was removed during integration; the final PR diff contains only the source and regression test.Validation on the current head
npm run buildandnpm run smoke:status-sourcespassed.cqr_15c78f13b991294deca3.The fallback affects only catalog persistence under storage denial. It does not grant remote write authority or change normal working-storage behavior.