Conversation
cyfung1031
force-pushed
the
improve/scripteditor-2
branch
from
da3f4cc to
5c4ef70
Compare
|
@claude can you please review this PR? |
claude需要付费，应该是不会的，我都是本地调用skill，这个pr做了什么改动？ 如果有必要，这个pr也进入稳定版本 |
CodFrm
left a comment
CodFrm
left a comment
There was a problem hiding this comment.
Code Review: 优化 ScriptEditor
变更: 12 文件, +301/-358
概述
这个 PR 对 ScriptEditor 组件进行了全面的重构和 UX 优化，主要包括：
- 类型抽取：将
install方法的参数/返回值抽取为TScriptInstallParam/TScriptInstallReturn - 编辑器状态管理重构：用
useRef+useCallback模式避免闭包陷阱，统一openScript逻辑 - Tab key 改为 UUID：TabPane 的 key 从数组 index 改为
script.uuid，更稳定 - 侧边栏 toggle 优化：从菜单移到 Tab 栏右侧按钮，状态持久化到 localStorage
- 视觉反馈增强：脚本列表根据打开/修改状态显示不同颜色
- URL 路由同步：用
navigate()替代setPageUrlSearchParams，编辑器切换时 URL 自动更新 - 离开提示优化：合并
confirm_leave_page和script_modified_leave_confirm，避免编辑器间导航时误弹提示 - 繁体中文注释统一为简体中文
问题和建议
1. 模块级变量 cid — 多实例风险
let cid: ReturnType<typeof setTimeout>;cid 定义在模块顶层，如果未来有多个 ScriptEditor 实例（虽然当前不太可能），会共享同一个 timer。建议改为 useRef：
const cidRef = useRef<ReturnType<typeof setTimeout>>();2. 模块级 DAO 实例
const scriptDAO = new ScriptDAO();
const scriptCodeDAO = new ScriptCodeDAO();从组件内移到模块顶层是合理的优化（避免每次渲染重建），但需要确认这些 DAO 是无状态的。如果 DAO 内部有缓存或副作用，可能会有问题。
3. hotKeys.current.length = 0 — 直接修改 ref 数组
const hotKeys = useRef<HotKey[]>([]);
hotKeys.current.length = 0;
menu.forEach((item) => { ... hotKeys.current.push(...) });每次渲染都清空并重建 hotKeys 数组。由于是原地修改同一个数组引用，如果其他地方持有这个引用（比如 openScript 中 hotKeys.current 传给了 EditorState），旧的 EditorState 中的 hotKeys 也会被清空。建议改为每次创建新数组：
hotKeys.current = [];4. handleDeleteEditor 中的潜在竞态
confirm() 阻塞和 await emptyScript() 期间，editorsRef.current 可能已变化，但 targetIndex 和 targetEditor 是快照值。setEditors 回调中使用了闭包中的 targetIndex 和 targetEditor.active，可能与实际 prev 不一致。建议在 setEditors 回调内部重新计算 index。
5. popstate 中硬编码 URL 路径
if (location.href.startsWith(chrome.runtime.getURL("/src/options.html#/script/editor"))) {
return;
}硬编码了 "/src/options.html#/script/editor"，如果构建输出路径变化会失效。建议提取为常量或动态获取。
6. filteredScriptList 渲染中的冗余查找
const scriptListScript = scriptList.find((v) => v.uuid === script.uuid);filteredScriptList 本身就是 scriptList.filter(...) 的结果，scriptListScript 永远等于 script 本身。这个 find 是多余的，直接用 script.status 即可。
7. 新增依赖 react-icons/vsc
需要确认 react-icons 已在 package.json 中声明。
8. openScript 依赖项的完整性
内部使用了 editorFindIndex、editorFindItem 等函数，通过 editorsRef.current 读取最新值所以实际不需要列入依赖。eslint 规则已被 suppress，逻辑上没问题但需要留意后续维护时的陷阱。
正面评价
- 重构后逻辑清晰很多，去除了大量重复代码（净减 57 行），消除了直接 mutate state 的反模式（如
editors[i].active = true） - Tab key 改为 UUID 是正确做法，避免了 index 变化导致的组件销毁重建
TScriptInstallReturn返回updatetime让前端保持数据一致性，设计合理- 侧边栏 toggle 按钮的 UX 比藏在菜单里好很多，localStorage 持久化也是正确的
- i18n 改进合理，合并了语义重复的翻译 key
总结
整体是高质量的重构 PR，显著改善了 ScriptEditor 的状态管理和用户体验。建议重点关注：
- 第3点：hotKeys ref 的 mutate 方式可能影响已创建的 EditorState
- 第4点：
handleDeleteEditor中的潜在竞态 - 第6点：冗余的
scriptListScript查找可以直接移除
There was a problem hiding this comment.
Pull request overview
This PR optimizes the ScriptEditor component in ScriptCat (a browser extension for user script management). The main changes refactor editor tab management to use UUID-based keys instead of array indices, fix a missing tw- CSS prefix on the group class that caused the delete button to be invisible, and improve the overall UI/UX of the script editor.
Changes:
- Refactored
ScriptEditorto use UUID-based tab keys,useReffor stable callbacks, and a unifiedopenScriptfunction, replacing the previous index-based and fragmented approach - Extended
installScriptAPI to returnupdatetimealongsideupdatestatus, and extracted shared types (TScriptInstallParam,TScriptInstallReturn) - Updated locale files: removed unused
confirm_leave_pagekey, improvedscript_modified_leave_confirmmessage wording, and normalized comments from Traditional to Simplified Chinese
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/pages/options/routes/script/ScriptEditor.tsx |
Major refactor: UUID-based tabs, openScript command pattern, useRef for editors state, CSS fixes (tw-group), editor state type extraction, improved tab close/activate logic |
src/pages/components/CustomLink/index.tsx |
Changed confirm dialog key from removed confirm_leave_page to script_modified_leave_confirm |
src/app/service/service_worker/script.ts |
Extracted TScriptInstallParam/TScriptInstallReturn types, installScript now returns updatetime, normalized comments to Simplified Chinese |
src/app/service/service_worker/client.ts |
Aligned install() method to use new shared types |
src/locales/en-US/translation.json |
Removed confirm_leave_page, updated script_modified_leave_confirm wording |
src/locales/zh-CN/translation.json |
Removed confirm_leave_page, updated script_modified_leave_confirm wording |
src/locales/zh-TW/translation.json |
Removed confirm_leave_page, updated script_modified_leave_confirm wording |
src/locales/ja-JP/translation.json |
Removed confirm_leave_page, updated script_modified_leave_confirm wording |
src/locales/de-DE/translation.json |
Removed confirm_leave_page, updated script_modified_leave_confirm wording |
src/locales/ru-RU/translation.json |
Removed confirm_leave_page, updated script_modified_leave_confirm wording |
src/locales/vi-VN/translation.json |
Removed confirm_leave_page, updated script_modified_leave_confirm wording |
src/locales/ach-UG/translation.json |
Removed confirm_leave_page (Crowdin placeholder locale) |
| const templateVal = useRef(pageUrlSearchParams.get("template")); | ||
| const targetVal = useRef(pageUrlSearchParams.get("target")); | ||
|
|
||
| // 袑始化 & 网址改变 |
There was a problem hiding this comment.
袑始化 应该是 初始化 的笔误（袑 不是正确的简体中文字符）。
| // 袑始化 & 网址改变 | |
| // 初始化 & 网址改变 |
| const scriptListScript = scriptList.find((v) => v.uuid === script.uuid); | ||
| const editor = editorFindItem(script.uuid); | ||
| const colorRGB = !editor ? "173,173,173" : editor.isChanged ? "230,155,31" : "199,199,199"; | ||
| const alpha = scriptListScript?.status === 2 ? 0.8 : 1.0; |
There was a problem hiding this comment.
scriptListScript 是多余的查找——filteredScriptList 已经是 scriptList.filter(...) 的结果，所以 script 本身就是 scriptList 中的对象，可以直接使用 script.status 来判断 alpha 值，无需再做一次 O(n) 的 scriptList.find()。
| const scriptListScript = scriptList.find((v) => v.uuid === script.uuid); | |
| const editor = editorFindItem(script.uuid); | |
| const colorRGB = !editor ? "173,173,173" : editor.isChanged ? "230,155,31" : "199,199,199"; | |
| const alpha = scriptListScript?.status === 2 ? 0.8 : 1.0; | |
| const editor = editorFindItem(script.uuid); | |
| const colorRGB = !editor ? "173,173,173" : editor.isChanged ? "230,155,31" : "199,199,199"; | |
| const alpha = script.status === 2 ? 0.8 : 1.0; |
| if (!rightOperationTab) return; | ||
|
|
||
| setEditors((prev) => { | ||
| const idx = editorFindIndex(rightOperationTab.uuid); |
There was a problem hiding this comment.
setEditors 的 updater 回调中使用了 editorFindIndex()（读取 editorsRef.current）来计算 idx，但 updater 回调的 prev 参数才是 React 保证的最新状态。如果有其他 setEditors 调用被 batch 在一起执行，editorsRef.current 可能和 prev 不一致，导致 idx 和 prev 的 index 不匹配。建议改为基于 prev 来查找 index，例如 const idx = prev.findIndex((e) => e.script.uuid === rightOperationTab.uuid);。
| const idx = editorFindIndex(rightOperationTab.uuid); | |
| const idx = prev.findIndex((e) => e.script.uuid === rightOperationTab.uuid); |
| const editors = editorsRef.current; | ||
| const targetIndex = editorFindIndex(targetUuid); | ||
| if (targetIndex === -1) return; | ||
| const targetEditor = editors[targetIndex]; | ||
|
|
||
| // 如果需要确认且脚本已修改 | ||
| if (needConfirm && targetEditor.isChanged) { | ||
| if (!confirm(t("script_modified_close_confirm"))) { | ||
| return prev; | ||
| } | ||
| } | ||
| // 如果需要确认且脚本已修改 | ||
| if (needConfirm && targetEditor.isChanged) { | ||
| if (!confirm(t("script_modified_close_confirm"))) return; | ||
| } | ||
|
|
||
| // 如果只剩一个编辑器，打开空白脚本 | ||
| if (prev.length === 1) { | ||
| const template = pageUrlSearchParams.get("template") || ""; | ||
| emptyScript(template || "", hotKeys, "blank").then((e) => { | ||
| setEditors([e]); | ||
| setSelectSciptButtonAndTab(e.script.uuid); | ||
| }); | ||
| return prev; | ||
| } | ||
| // 如果只剩一个编辑器，打开空白脚本 | ||
| if (editors.length === 1) { | ||
| const template = templateVal.current || ""; | ||
| const e = await emptyScript(template || "", hotKeys.current, "blank"); | ||
| setEditors([e]); | ||
| setSelectSciptButtonAndTab(e.script.uuid); | ||
| return; | ||
| } | ||
|
|
||
| setEditors((prev) => { | ||
| // 删除目标编辑器 | ||
| prev = prev.filter((_, index) => index !== targetIndex); | ||
|
|
||
| const filtered = prev.filter((e) => e.script.uuid !== targetUuid); | ||
| // 如果删除的是当前激活的编辑器，需要激活其他编辑器 | ||
| if (targetEditor.active && prev.length > 0) { | ||
| let nextActiveIndex; | ||
| if (targetIndex >= prev.length) { | ||
| // 如果删除的是最后一个，激活前一个 | ||
| nextActiveIndex = prev.length - 1; | ||
| } else { | ||
| // 否则激活下一个（原来的下一个现在在同样的位置） | ||
| nextActiveIndex = targetIndex; | ||
| } | ||
| prev[nextActiveIndex].active = true; | ||
| setSelectSciptButtonAndTab(prev[nextActiveIndex].script.uuid); | ||
| // 如果删除的是最后一个，激活前一个 | ||
| // 否则激活下一个（原来的下一个现在在同样的位置） | ||
| const nextActiveIndex = targetIndex >= filtered.length ? filtered.length - 1 : targetIndex; | ||
| filtered[nextActiveIndex] = { ...filtered[nextActiveIndex], active: true }; | ||
| setSelectSciptButtonAndTab(filtered[nextActiveIndex].script.uuid); |
There was a problem hiding this comment.
和上面的 rightOperationTab effect 类似，handleDeleteEditor 在 setEditors((prev) => ...) 外部通过 editorsRef.current 获取了 targetIndex 和 targetEditor（第 635-637 行），然后在 updater 内部使用了这些外部值（如 targetEditor.active 和 targetIndex）。如果在 confirm() 阻塞期间或 await emptyScript() 期间有其他状态更新被触发，prev 可能和 editorsRef.current 不一致，导致 targetIndex 指向错误的 tab。建议将 index 查找和 active 判断移到 updater 内部，基于 prev 来计算。
| // 恢复标题 | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [canLoadScript, pageUrlSearchParams, pageUrlParams.uuid, navigate]); |
There was a problem hiding this comment.
navigate 出现在依赖数组中但未在 effect 内部使用。虽然 useNavigate() 返回的是稳定引用不会导致额外的 re-run，但这会让读者产生困惑。建议从依赖数组中移除 navigate。另外，这个 effect 缺少 openScript、selectedScript、scriptList 等实际使用的依赖（虽然有 eslint-disable 注释）。如果有意省略这些依赖，建议在注释中说明原因。
| type visibleItem = "scriptStorage" | "scriptSetting" | "scriptResource"; | ||
|
|
||
| const popstate = () => { | ||
| let cid: ReturnType<typeof setTimeout>; |
There was a problem hiding this comment.
cid 是模块级变量，所有 ScriptEditor 组件实例共享同一个 timeout ID。在 React Strict Mode（开发环境下）组件可能被双重挂载/卸载，这可能导致一个实例的 clearTimeout 清掉另一个实例设置的 timeout。建议将 cid 改为 useRef 以保证每个组件实例有独立的 timeout。同样的问题也存在于 scriptDAO 和 scriptCodeDAO（第 201-202 行），虽然 DAO 是无状态的所以影响较小。
| color: "var(--color-text-2)", | ||
| color: `${delBtnRGBA}`, | ||
| boxShadow: "none", | ||
| position: "absolute", |
There was a problem hiding this comment.
position: "absolute" 在 inline style（第 953 行）和 className tw-absolute（第 944 行）中重复声明。建议移除其中一个以避免冗余。
| position: "absolute", |
- 将模块级 cid 变量改为 useRef，避免多实例共享 timer - hotKeys.current.length = 0 改为 hotKeys.current = []，避免原地修改影响旧引用 - handleDeleteEditor 在 setEditors 回调内重新计算 index，修复竞态问题 - 移除冗余的 scriptList.find 查找 - 修复「袑始化」笔误为「初始化」 - 移除重复的 position: absolute 声明
原來如此。我只是想試一下能不能用 |
原来 1.3 之前加
tw-那个， "group" 加漏了。一直没人发现删除按钮不见了哈哈优化一下
你实际操作一下就会感到不一样
上一页下一页弹关闭提醒那个太麻烦了。我没有改过
这个PR主要是 React UI/UX