✨ Popup 站点范围操作:S1–S4 可逆状态机(#1590) - #1666
Conversation
cyfung1031
left a comment
There was a problem hiding this comment.
@CodFrm 我按当前 head 174025cac456b6a859d1927e4561268cfe274662,结合 #1590、已合并的 #1646 以及 service worker / popup / 设置页的实际链路走了一遍。除了 UI 状态机本身,下面几处建议在合入前处理:
-
空 match 覆盖没有清掉持久化的 CompiledResource 和浏览器注册。
excludeFromMatch删除最后一项后会保存selfMetadata.match = [](script.ts#L967-L980)。这会让applyScriptMatchInfo清掉内存 matcher,但它只删除cachedPatterns(runtime.ts#L1692-L1700)。随后updateResourceOnScriptChange在 build 返回 undefined 时直接 return(runtime.ts#L452-L460、runtime.ts#L844-L850),没有 unregister 旧的chrome.userScripts,也没有删除旧的CompiledResource。而 SW 重启时waitInit会直接信任旧的 compiled resource 并重新加回旧规则(runtime.ts#L393-L415)。因此“空覆盖=全站不匹配”目前只在当前内存实例成立,重启后旧范围可能复活。现有测试只断言内存 matcher,建议补上持久化/重启/实际注册状态,并在空结果路径显式删除 compiled resource、注销旧注册。 -
onlyRunOnUrl清空include,但设置页的 reset 只恢复match。 这里新增了selfMetadata.include = [](script.ts#L929-L936),但设置页的重置最终只调用resetMatch,后端也只删除selfMetadata.match(script.ts#L1007-L1016)。对于原本只靠@include匹配的脚本,执行“仅在本站”后再从设置页重置 match,遗留的include: []会让有效 metadata 没有任何 URL 规则(utils.ts#L244-L273),用户无法通过这个 reset 路径恢复原行为。建议把这个动作的 match/include 覆盖作为一个可原子撤销的 scope,至少补上 only-run → settings reset → 原始 include 恢复的回归测试。 -
状态契约只检查
selfMetadata.match,会漏掉 include-only 覆盖。 types.ts#L200-L202 和 popup.ts#L393-L406 把hasMatchOverride定义为只看match;但如果存在selfMetadata.include而没有match,popup 会把它当成 S1/S2。此时allowUrl也只在已有 match 覆盖时加 match、只在已有 exclude 覆盖时删 exclude(script.ts#L950-L960),可能出现点击后返回成功但实际 include 范围没有改变。建议明确 scope 覆盖是 match/include 的联合状态,或明确禁止/迁移 include-only 状态。 -
excludeFromMatch在已有用户 exclude 覆盖时会丢掉作者@exclude。 这段使用selfMetadata.exclude || script.metadata.exclude二选一(script.ts#L971-L978),而 metadata 合并对同名 key 是用户值整体替换作者值(utils.ts#L139-L147)。例如作者有@exclude author、用户已有exclude user时,再排除本站会保存[user, current],作者规则就不再生效。这也暴露了原有excludeUrl的同类问题,但本 PR 明确宣称保护作者@exclude,所以建议至少为这个新路径补“作者规则 + 已有用户规则”的组合测试,并统一合并语义。
|
🤣 没有人类了 @cyfung1031 感谢详细走查,四条都已处理,落在提交 1. 空 match 覆盖没有清掉持久化 CompiledResource 和浏览器注册 确实只清了内存 matcher。已在 2. onlyRunOnUrl 清 include,但设置页 reset 只恢复 match 按建议把 match/include 覆盖作为一个可原子撤销的 scope: 3. 状态契约只检查 match,漏 include-only 采用「明确禁止 include-only 状态」路线:第 2 点让 include-only 无法再产生——include 覆盖的唯一来源 4. excludeFromMatch 已有用户 exclude 覆盖时丢作者 @exclude 已统一合并语义: 全量 Vitest 327 文件 / 3713 测试全绿;typecheck / eslint / prettier / i18n 检查均通过。 |
全量扫描(12 模板 YAML 解析 + 全量 src 预填契约 TS AST 遍历,冷启动 ~200ms)原来塞在一个受 fast 项目 340ms 单元预算约束的 it 里,CI 并行 负载下必然偶发 Test timed out in 340ms。把「计算」移进 beforeAll (vitest hook 走默认 10s 预算,与 testTimeout 无关)并缓存结果, 断言阶段的 it 只做快照校验;行为与 CLI check:issue-templates 不变。
cyfung1031
left a comment
There was a problem hiding this comment.
@CodFrm 我按最新 head 42ed69f2167e06aa33193d35c9b60b8f1986d0be 重新复核了一遍。前面四项修复已确认;901e3505 之后的新 commit 只重构 issue template 检查,不影响站点范围链路。但下面仍有问题,暂不建议合入:
-
[P1]
allowUrl在已有用户 exclude 覆盖时仍会丢作者@exclude。script.ts#L957-L961只从selfMetadata.exclude建 Set;例如作者@exclude=[author]、用户覆盖为[current,user]时,允许当前站点会保存[user]。由于getCombinedMeta对同名 key 是用户值整体替换作者值(utils.ts#L139-L147),作者规则随后不再生效。这与 PR 描述中“allowUrl 不改写作者 @exclude”不一致;当前新增测试只覆盖“没有用户 exclude 覆盖”的情况。建议与另外两条路径一样合并metadata.exclude ∪ selfMetadata.exclude后再删除当前项,并补上“作者规则 + 已有用户规则”的allowUrl回归测试。 -
[P1] 站点范围读改写的队列边界仍不完整。
onlyRunOnUrl、allowUrl、excludeFromMatch使用script-site-scope,但同样读出完整脚本后写回selfMetadata的excludeUrl、resetMatch、resetExclude没有入队(script.ts#L899-L1037)。例如 popup 的onlyRunOnUrl和设置页的resetMatch并发时,二者都从旧快照读取,最后一次scriptDAO.update(uuid, script)会覆盖另一次操作;DAO 的update只是对存储对象做浅层Object.assign(repo.ts#L322-L341),不会合并嵌套的selfMetadata。这会让“仅运行在本站”或设置页重置偶发丢失。建议把所有 URL scope 的读改写统一放进同一队列,或改为原子字段更新,并补跨 popup/设置页入口的并发回归测试。 -
[P2] “include-only 状态不可产生”目前只是约定,没有被代码强制。
updateMetadata的 client 和 service-worker handler 都接受任意key: string(client.ts#L150-L152、script.ts#L1613-L1627),因此协议仍可写入只有selfMetadata.include而没有match的状态;已有导入/历史数据也可能带着这种状态。可是hasMatchOverride和 popup 判定仍只检查selfMetadata.match(types.ts#L203-L204、popup.ts#L398-L399),而resetMatch(undefined)又会无条件删除include。如果 include-only 确实是禁止状态,请在写入边界校验/归一化并覆盖历史数据;否则应把 match/include 作为联合 scope 状态计算,并区分 onlyRun 写入的 include 覆盖与独立 include 覆盖。
以上不是 GitHub 的机械 mergeability 问题,而是运行时行为和状态一致性问题。
|
@CodFrm 测试没有针对确切问题来做 |
cyfung1031
left a comment
There was a problem hiding this comment.
@CodFrm 我已按 PickInvariant 绑定精确 head 30a1f211130bc155bb6cd371bf8e722932a2513a,并让独立 subagents 分别检查每个修复提交。之前指出的三类问题现在已验证修好:作者 @exclude 保留、所有 metadata read-modify-write 进入同一队列、onlyRun 的 include: [] 与独立 include-only 覆盖可区分。当前本地针对相关 4 个测试文件为 141/141 通过;GitHub test workflow 尚在运行。
仍建议保留以下设计审查项:
-
[P2] Popup 删除了“已运行但当前 URL 不匹配”的 live script 回填。
popup.ts:getPopupData现在只把matchingResult放入scriptList,而父版本原本会从tabScript:<tabId>缓存补回仍存在于 DAO 的脚本。PR 新增的测试只用 DAO 返回空/已删除脚本,未覆盖“缓存有 live script、DAO 仍有该脚本、当前 URL 不匹配”的对照;因此测试锁定了删除残留,却没有证明删除 live fallback 是预期行为。若 popup 仍应让用户找到刚运行过但当前不匹配的脚本,建议恢复带 DAO 防护的回填;若这是刻意改动,请补充产品语义和 live-script 回归测试。 -
[P2]
script-site-scope是全局队列 key。 不同 UUID 的站点操作也会互相阻塞;当前测试只证明同一脚本的顺序,没有证明跨脚本可并行。若目标只是避免同一脚本的 stale snapshot,建议使用script-site-scope:<uuid>(或明确说明全局串行是刻意的吞吐/一致性取舍)。 -
[P2] 作者排除规则被 materialize 到 selfMetadata 后会冻结作者后续更新。
excludeUrl、allowUrl、excludeFromMatch为避免整体替换而保存metadata.exclude ∪ selfMetadata.exclude;之后脚本作者新增/删除@exclude时,旧的用户快照仍会整体覆盖作者值。PR body 已列为 known limitation,但这会使“保护作者规则”与“跟随作者更新”互相冲突;长期更稳的是保存用户 delta/tombstone,或至少补一个“操作后作者 metadata 变化”的行为测试。 -
[P2] provenance marker 目前寄存在开放的 SCMetadata 命名空间。
__scriptcat_only_run_on_url已在当前getCombinedMeta中过滤,这是可行的短期修复;但它会随 backup/sync 持久化,旧版本或任意updateMetadata(key)路径可能把它当普通 metadata。长期建议使用 typed scope/provenance 字段,或明确保留 internal-key 的跨版本兼容契约。
这些是运行时状态/数据语义问题,不是 GitHub 的 mergeability 问题;当前 PR 的 CI/checks 仍应独立看待。
感觉是两个方向,TDD也是为了防止问题回归 AI review出来的问题,太过于兜底稳健了,不知道如何评价,我一般只会做一轮review,处理那些显而易见的问题 PickInvariant 我下次试试 |
再这么review处理下去,不知道要改多少了,已经超出这个pr的内容了,所以我很不喜欢review多轮,AI总能查出问题 |
这个 skill 有界的。那4个点它不坚持走下去。 就只是跟你说一下。 还是交给你处理吧
|
* 🐛 popup 当前页脚本按实际注入展示,而非 pattern 命中 (#1687) popup「当前页运行脚本」原本只按顶层网址做 pattern 匹配,带来两类失真: 1. iframe 内运行的脚本整条消失。#1666 为了让「排除本站」后该行立即消失, 删掉了 #1511 加的「运行过但未匹配」合并,代价是 @match 只命中 iframe 的 脚本连同它在 iframe 注册的 GM 菜单都不再出现。现改为「本 tab 跑过 ∧ 仍匹配 某个子 frame」——排除本站后脚本对所有 frame 都不再匹配,仍会立即消失。 仅匹配子 frame 的行标记 matchesTopFrame=false,UI 据此隐藏按顶层 host 生成规则的站点范围操作(否则会写出 *://settings/* 这类垃圾规则)。 2. chrome:// 等脚本猫触及不到的页面上照样列出脚本。新增页面状态判定: 协议/商店名单给出准确原因,「本 tab 有没有 content script 报到」作为 运行时证据兜住白名单漏掉的情况(企业策略等)。file:// 的权限查询只用于 给未注入的情况一个更准确的原因,不反过来否定已注入的事实。 黑名单页并入同一状态——它同样不会注入,此前却照常列脚本。 GetPopupDataRes.isBlacklist 相应替换为 pageStatus。 e2e/popup-matching-regressions.spec.ts 原先用不授予 userScripts 权限的 fixture,脚本从未真正注入,验证的只是 pattern 命中这一层(即本 issue 的假象 在测试套件里的镜像)。改用带权限与 .test host 解析的 fixture + 本地 mock 页。 * 🐛 扩展商店判定移到注入证据之后,并补上 Edge 商店 商店域是浏览器相关的:Edge 商店在 Chrome 里就是普通网页,Chrome 商店在 Firefox 里也一样。原实现把商店域并进 getPageAccessKind 的硬名单、优先于 注入证据判定,会在「别家浏览器」上误报脚本不能运行——而它其实在跑。 改为与 file:// 权限查询同一处理:只用来给「已确认没注入」的页面一个更准确 的原因,不反过来否定已注入的事实。顺带补上 microsoftedge.microsoft.com/addons。 * 🐛 popup 只显示实际注入的脚本 * Revert "🐛 popup 只显示实际注入的脚本" This reverts commit 67270db. * 🐛 popup 只显示实际注入的脚本 * 🐛 丢弃过期的 Popup 页面运行记录 * Revert "🐛 丢弃过期的 Popup 页面运行记录" This reverts commit 4253a26. * Revert "🐛 popup 只显示实际注入的脚本" This reverts commit bac5f1c. * 🐛 popup 可达性判定修正:bfcache 还原与「脚本功能未启用」不再误报 评审实测(真实 Chrome,main / PR 两边逐场景对照)发现「本 tab 有没有 content script 报到」这条判据有三处会给出错误结论,本次逐条修正。 1. bfcache 还原后误报「脚本未运行」。 src/scripting.ts 的 pageLoad() 只在 document_start 执行一次,而 bfcache 还原不会重新注入 content script,因此不会再上报。跨 origin 后退时 tabLoaded 仍停在离开的那个 origin,整页脚本连同它注册的 GM 菜单被判为没在 跑——而它们随文档一起被恢复,实际都还活着。 新增 pageShow 上报(仅顶层 frame、仅 event.persisted)→ 新 mq 事件 popupPageRestored → PopupService.markTabInjected 补回注入标记。它只重新确认 可达性,不重放脚本、不重复计运行次数。 实测:后退回上一页由 not-injected / 0 条恢复为 ok / 1 条。 2/3. 全局「启用脚本」关闭、UserScripts API 不可用(未开开发者模式等)时, registerUserscripts() 直接 return,scriptcat-scripting 根本没注册,页面永远 不会上报,于是一律落到 not-injected「刷新页面后生效」——而刷新永远不会生效。 新增 scripts-disabled / userscripts-unavailable 两个状态给出真实原因。两者 仍排在注入证据之后:关掉开关不会杀死已注入页面上正在跑的脚本,那些页照旧为 ok。 实测:not-injected → scripts-disabled / userscripts-unavailable。 顺带补齐 pageStatus 改名遗留的测试替身(preload.test / usePopupData.test / chrome-extension-mock)与 page_access.ts 中与所标注常量语义相反的注释。 单测 343 文件 / 4267 条全过;pnpm run lint 全 0;全量 e2e 62 passed。 --------- Co-authored-by: cyfung1031 <44498510+cyfung1031@users.noreply.github.com>

Checklist / 检查清单
背景
#1590 中 PR #1646 引入的 popup 站点范围快捷操作存在三个问题:①「仅运行在 xxx」是
resetMatch整表替换,一次点击静默清空全部原始匹配规则、且弹窗内不可撤销;②同一脚本在不同站点显示语义相反的按钮(仅运行在 ↔ 包含);③白名单单向不可逆,「排除」让同一站点同时出现在设置页「网站匹配」与「网站排除」两张表。本次改动
把弹窗站点范围操作收敛为 S1–S4 可逆状态机:
excludeFromMatch:移出 match + 加入用户 exclude)ScriptMenu.hasMatchOverride,popup 依isEffective × hasMatchOverride分类四态。allowUrl/excludeFromMatch只增删用户覆盖、绝不折叠/改写作者@exclude;仅运行在同时清空@match与@include;三个站点操作经静态队列串行化,避免并发读改写丢更新;空匹配覆盖(排除最后一项)时清空残留 matcher 规则(spec 决策 5「空覆盖=全站不匹配」)。实现考虑
@exclude规则不可被弹窗操作改写;excludeFromMatch为保证不丢作者规则会把作者@exclude并入用户覆盖。已知限制
excludeFromMatch把作者@exclude复制进用户覆盖(保留作者规则,但用户覆盖冻结了作者规则快照)。.gitignore忽略docs/specs/,以本地工件形式存在,未提交进 git。测试