Conversation
There was a problem hiding this comment.
嗨,我在这里给了一些总体反馈:
- 在
handlePreActionDragEnd和reorderPreActions中,没有对findIndex返回-1的情况进行保护,因此如果出现异常的拖拽事件(DnD event),可能会在索引-1处执行splice。建议在调用reorderPreActions或修改数组之前,当任一索引< 0时提前返回。 - 在
InlineNameEditor中,你使用了React.KeyboardEvent,但文件中没有导入 React 的类型;请添加import type React from 'react';(或者更改事件类型),以避免在更严格的 TS 配置中出现类型错误。 updatePreAction/removePreAction/renamePreAction这几个 store 方法依赖已有的preActions数组且没有兜底逻辑。为了更安全,你可能需要显式处理undefined/空数组的情况(例如通过提前返回),以避免不必要的对象拷贝,或者在这些方法在尚未添加任何 action 之前被调用时出现undefined.map之类的问题。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- In `handlePreActionDragEnd` and `reorderPreActions`, there’s no guard for cases where `findIndex` returns `-1`, so a malformed DnD event could splice at `-1`; consider early-returning when either index is `< 0` before calling `reorderPreActions` or mutating the array.
- In `InlineNameEditor` you reference `React.KeyboardEvent` but the file doesn’t import React’s types; add `import type React from 'react';` (or change the event type) to avoid type errors in stricter TS configurations.
- The `updatePreAction`/`removePreAction`/`renamePreAction` store methods rely on existing `preActions` arrays without fallback, so for safety you may want to explicitly handle the `undefined`/empty case (e.g., by early-returning) to avoid unnecessary object copies or accidental `undefined.map` if these are ever called before any actions are added.帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've left some high level feedback:
- In
handlePreActionDragEndandreorderPreActions, there’s no guard for cases wherefindIndexreturns-1, so a malformed DnD event could splice at-1; consider early-returning when either index is< 0before callingreorderPreActionsor mutating the array. - In
InlineNameEditoryou referenceReact.KeyboardEventbut the file doesn’t import React’s types; addimport type React from 'react';(or change the event type) to avoid type errors in stricter TS configurations. - The
updatePreAction/removePreAction/renamePreActionstore methods rely on existingpreActionsarrays without fallback, so for safety you may want to explicitly handle theundefined/empty case (e.g., by early-returning) to avoid unnecessary object copies or accidentalundefined.mapif these are ever called before any actions are added.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handlePreActionDragEnd` and `reorderPreActions`, there’s no guard for cases where `findIndex` returns `-1`, so a malformed DnD event could splice at `-1`; consider early-returning when either index is `< 0` before calling `reorderPreActions` or mutating the array.
- In `InlineNameEditor` you reference `React.KeyboardEvent` but the file doesn’t import React’s types; add `import type React from 'react';` (or change the event type) to avoid type errors in stricter TS configurations.
- The `updatePreAction`/`removePreAction`/`renamePreAction` store methods rely on existing `preActions` arrays without fallback, so for safety you may want to explicitly handle the `undefined`/empty case (e.g., by early-returning) to avoid unnecessary object copies or accidental `undefined.map` if these are ever called before any actions are added.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
该 PR 旨在将“前置程序/前置任务”从单个配置升级为支持多个条目,并补齐排序、复制、重命名等配套能力,同时对配置结构做向后兼容迁移。
Changes:
- 将
preAction迁移为preActions[](含id/customName),并在 store 中提供增删改/排序/复制/重命名能力及兼容迁移逻辑 - UI 支持前置程序列表展示与拖拽排序,运行前按顺序依次执行多个前置程序,并增强日志提示(带名称)
- 抽取 Task/Action 共用的右键菜单与内联重命名组件,减少重复实现
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/interface.ts | ActionConfig 增加 id/customName,Instance 改为 preActions[] |
| src/types/config.ts | 增加 LegacyActionConfig,并为已保存配置提供 preActions + deprecated preAction 兼容字段 |
| src/stores/types.ts | 更新 store API:新增前置程序的增删改/排序/复制/重命名方法签名 |
| src/stores/appStore.ts | 实现 preAction -> preActions 迁移与新增的前置程序管理方法,并在加载/保存/复制/恢复流程接入 |
| src/components/Toolbar.tsx | 启动任务前按列表顺序执行多个前置程序,新增带名称的日志文案 |
| src/components/TaskList.tsx | 前置程序列表渲染与拖拽排序接入 |
| src/components/TaskItem.tsx | 复用抽取后的菜单/内联编辑组件以减少重复代码 |
| src/components/listItemShared.tsx | 新增共享:右键菜单构建器 + 内联重命名编辑器 |
| src/components/AddTaskPanel.tsx | 支持多次添加前置程序(每次生成新 id) |
| src/components/ActionItem.tsx | 前置程序条目 UI:支持拖拽、右键菜单、复制/重命名/删除确认等能力 |
| src/i18n/locales/*.ts | 增加前置程序命名执行提示与前置程序右键菜单相关文案 |
| const handlePreActionDragEnd = (event: DragEndEvent) => { | ||
| if (isInstanceRunning) return; | ||
| const { active, over } = event; | ||
| if (over && active.id !== over.id && instance?.preActions) { | ||
| const oldIndex = instance.preActions.findIndex((a) => a.id === active.id); | ||
| const newIndex = instance.preActions.findIndex((a) => a.id === over.id); | ||
| reorderPreActions(instance.id, oldIndex, newIndex); | ||
| } |
There was a problem hiding this comment.
findIndex can return -1 if the dragged id isn't found (e.g. list changed during drag, or stale DnD state). Calling reorderPreActions with -1 will reorder the wrong item (because splice(-1, 1) removes the last element). Add a guard to ensure both indices are >= 0 before reordering (and optionally no-op when oldIndex === newIndex).
| const [removed] = items.splice(oldIndex, 1); | ||
| items.splice(newIndex, 0, removed); |
There was a problem hiding this comment.
reorderPreActions assumes oldIndex/newIndex are valid. If either index is out of range (or -1), splice will remove/insert the wrong element and can even insert undefined into the list. Consider validating bounds (0 <= idx < length), clamping newIndex, and early-returning when indices are invalid or equal.
| const [removed] = items.splice(oldIndex, 1); | |
| items.splice(newIndex, 0, removed); | |
| const itemCount = items.length; | |
| if ( | |
| itemCount < 2 || | |
| oldIndex < 0 || | |
| oldIndex >= itemCount || | |
| oldIndex === newIndex | |
| ) { | |
| return i; | |
| } | |
| const clampedNewIndex = Math.max(0, Math.min(newIndex, itemCount - 1)); | |
| const [removed] = items.splice(oldIndex, 1); | |
| if (!removed) return i; | |
| items.splice(clampedNewIndex, 0, removed); |
| title={t('taskItem.removeConfirmTitle')} | ||
| message={t('taskItem.removeConfirmMessage')} |
There was a problem hiding this comment.
The delete confirmation dialog reuses taskItem.removeConfirmTitle/message, which currently reads as “Delete task”. When deleting a pre-action, this produces incorrect/misleading UI copy. Add dedicated i18n keys for pre-action deletion (or use a generic delete confirmation string) and reference those here.
| title={t('taskItem.removeConfirmTitle')} | |
| message={t('taskItem.removeConfirmMessage')} | |
| title={t('actionItem.removeConfirmTitle', { defaultValue: 'Delete pre-action' })} | |
| message={t('actionItem.removeConfirmMessage', { | |
| defaultValue: 'Are you sure you want to delete this pre-action?', | |
| })} |
Summary by Sourcery
为每个实例支持多个可配置的预操作,并在连接前按顺序执行,同时改进管理和用户体验。
New Features:
Enhancements:
Original summary in English
Summary by Sourcery
Support multiple configurable pre-actions per instance and execute them in order before connecting, with improved management and UX.
New Features:
Enhancements: