feat(skills): allow replacing non-builtin skill auth without re-uploa… - #2223
Conversation
…ding The replace dialog for non-builtin skills now defers submission until the user clicks OK, so they can update only the managing team and visibility without re-selecting an archive. Builtin skills keep the existing pick-to-upload flow. The detail panel also surfaces the managing team and visibility in its meta section.
📝 WalkthroughWalkthroughThe Skill management flow now supports optional-file replacement, authorization-aware updates, team and visibility metadata in the detail panel, and expanded upload-modal translations across five locales. ChangesSkill management flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UploadSkillModal
participant SkillDetailPanel
participant List
User->>UploadSkillModal: select archive or confirm without file
UploadSkillModal->>SkillDetailPanel: submit file and authorization
SkillDetailPanel->>List: import or update skill
List->>List: refresh skill and show result
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the AI Skills UI to support replacing non-builtin skills’ auth settings (managing team + visibility) without requiring the user to re-upload an archive, and surfaces managing team + visibility in the skill detail meta section.
Changes:
- Update
UploadSkillModalto support submitting auth changes with an optional file (for non-builtin replace), while keeping builtin skills’ “pick-to-upload” flow. - Extend import/replace handlers to accept
File | undefined, and add an auth-only update path when no file is provided. - Display managing team and visibility in
SkillDetailPanelmeta.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/aiConfig/skills/pages/UploadSkillModal.tsx | Adds optional-file submission + confirm-based flow for auth fields; keeps builtin immediate upload flow. |
| src/pages/aiConfig/skills/pages/SkillSidebar.tsx | Updates onImport typing to accept `File |
| src/pages/aiConfig/skills/pages/SkillDetailPanel.tsx | Fetches team list and renders managing team + visibility in meta; adjusts replace menu icon/text. |
| src/pages/aiConfig/skills/pages/List.tsx | Supports auth-only replace (PUT) when no file is provided; updates success/error messaging. |
| src/pages/aiConfig/skills/locale/zh_HK.ts | Adds new strings for auth-only save and file-required validation. |
| src/pages/aiConfig/skills/locale/zh_CN.ts | Adds new strings for auth-only save and file-required validation. |
| src/pages/aiConfig/skills/locale/ru_RU.ts | Adds new strings for auth-only save and file-required validation. |
| src/pages/aiConfig/skills/locale/ja_JP.ts | Adds new strings for auth-only save and file-required validation. |
| src/pages/aiConfig/skills/locale/en_US.ts | Adds new strings for auth-only save and file-required validation. |
| onOk={handleConfirm} | ||
| okText={t('common:btn.ok')} | ||
| cancelText={t('common:btn.cancel')} | ||
| confirmLoading={submitting} | ||
| footer={showAuthFields ? undefined : null} | ||
| width={980} |
| React.useEffect(() => { | ||
| if (!item.user_group_ids) { | ||
| setUserGroups([]); | ||
| return; | ||
| } | ||
| getTeamInfoList() | ||
| .then((res) => { | ||
| setUserGroups(res.dat ?? []); | ||
| }) | ||
| .catch(() => { | ||
| setUserGroups([]); | ||
| }); | ||
| }, [item.user_group_ids]); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/aiConfig/skills/pages/List.tsx (2)
171-204: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRefresh failure is misreported as a save failure, risking duplicate non-idempotent writes on retry.
Both
handleImportandhandleUpdateImportwrap the actual write (importItem/importItemToUpdate/putItem) together with the follow-up refresh (run()/refreshSkill()) in one try/catch. If the write succeeds but only the refresh call fails, the catch shows an error toast and rethrows, causingUploadSkillModal.submitFileto keep the modal open for a retry — even though the save already succeeded. RetryinghandleImportin particular resubmitsimportItem, a presumably non-idempotent create call, risking a duplicate skill.🛡️ Suggested fix
async function handleImport(file: File | undefined, auth: SkillAuthValues) { if (!file) { return; } try { await importItem(file, auth); - await run(); - message.success(t('upload_file_success')); } catch (_error) { message.error(t('upload_file_error')); throw _error; } + message.success(t('upload_file_success')); + try { + await run(); + } catch (error) { + console.error(error); + } } async function handleUpdateImport(skillId: number, file: File | undefined, auth: SkillAuthValues) { try { if (file) { await importItemToUpdate(skillId, file, auth); } else { const currentSkill = await getItem(skillId); await putItem(skillId, { ..._.pick(currentSkill, ['name', 'description', 'instructions', 'license', 'compatibility', 'allowed_tools', 'metadata']), enabled: currentSkill.enabled, user_group_ids: auth.user_group_ids ?? currentSkill.user_group_ids, private: auth.private ?? currentSkill.private ?? 1, }); } - await refreshSkill(skillId); - message.success(file ? t('upload_file_success') : t('common:success.modify')); } catch (_error) { message.error(file ? t('upload_file_error') : t('modify_error')); throw _error; } + message.success(file ? t('upload_file_success') : t('common:success.modify')); + try { + await refreshSkill(skillId); + } catch (error) { + console.error(error); + } }🤖 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/pages/aiConfig/skills/pages/List.tsx` around lines 171 - 204, Separate the write operations from the follow-up refresh calls in handleImport and handleUpdateImport. Keep importItem, importItemToUpdate, and putItem inside the save error handling so write failures still show the existing save errors, but run run() or refreshSkill() after a successful save without rethrowing it as a save failure or keeping the upload modal in retry state; preserve the existing success messaging for completed writes.
451-453: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
returnbreaks the new "keep modal open on failure" retry safety.This wrapper synchronously returns
undefinedinstead of the promise fromhandleUpdateImport. BecauseUploadSkillModal.submitFiledoesawait onSubmit(file, auth), the await resolves immediately and the modal closes (state cleared,onCancel()called) before the actual PUT/import call even completes — regardless of success or failure. This silently defeats the new retry-on-error behavior added inUploadSkillModal.tsx(the modal is designed to stay open on failure so the user can retry, but here it closes unconditionally).SkillSidebar.tsx'sonSubmit={onImport}avoids this by passing the real async function reference directly.🐛 Suggested fix
- onImport={(file, auth) => { - handleUpdateImport(selectedSkillData.id, file, auth); - }} + onImport={(file, auth) => handleUpdateImport(selectedSkillData.id, file, auth)}🤖 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/pages/aiConfig/skills/pages/List.tsx` around lines 451 - 453, Return the promise from handleUpdateImport in the onImport wrapper in List.tsx by making the callback expression propagate its result. Preserve the existing selectedSkillData.id, file, and auth arguments so UploadSkillModal.submitFile can await completion and keep the modal open when the import fails.
🧹 Nitpick comments (1)
src/pages/aiConfig/skills/pages/SkillDetailPanel.tsx (1)
64-77: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
getTeamInfoList()calls due to array-reference dependency.
item.user_group_idsis a fresh array reference on everygetItem/refreshSkillfetch, even when its contents are unchanged. Since this effect depends on that reference, it re-fetches the entire team list on every skill switch and every detail refresh (toggle, replace, update), not just when relevant data actually changes.SkillAuthFields.tsxfetches the same list once with an empty dependency array — consider following that pattern here instead of tying it toitem.user_group_ids.♻️ Suggested fix
React.useEffect(() => { - if (!item.user_group_ids) { - setUserGroups([]); - return; - } getTeamInfoList() .then((res) => { setUserGroups(res.dat ?? []); }) .catch(() => { setUserGroups([]); }); - }, [item.user_group_ids]); + }, []);🤖 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/pages/aiConfig/skills/pages/SkillDetailPanel.tsx` around lines 64 - 77, Update the React.useEffect in SkillDetailPanel to avoid depending on the array reference item.user_group_ids, which changes on every refresh. Follow the existing SkillAuthFields pattern by fetching getTeamInfoList once with an empty dependency array, while preserving the current success, error, and missing-data handling.
🤖 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.
Outside diff comments:
In `@src/pages/aiConfig/skills/pages/List.tsx`:
- Around line 171-204: Separate the write operations from the follow-up refresh
calls in handleImport and handleUpdateImport. Keep importItem,
importItemToUpdate, and putItem inside the save error handling so write failures
still show the existing save errors, but run run() or refreshSkill() after a
successful save without rethrowing it as a save failure or keeping the upload
modal in retry state; preserve the existing success messaging for completed
writes.
- Around line 451-453: Return the promise from handleUpdateImport in the
onImport wrapper in List.tsx by making the callback expression propagate its
result. Preserve the existing selectedSkillData.id, file, and auth arguments so
UploadSkillModal.submitFile can await completion and keep the modal open when
the import fails.
---
Nitpick comments:
In `@src/pages/aiConfig/skills/pages/SkillDetailPanel.tsx`:
- Around line 64-77: Update the React.useEffect in SkillDetailPanel to avoid
depending on the array reference item.user_group_ids, which changes on every
refresh. Follow the existing SkillAuthFields pattern by fetching getTeamInfoList
once with an empty dependency array, while preserving the current success,
error, and missing-data handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ed2c25d-7885-456e-bf77-ba8cb5f68a83
📒 Files selected for processing (9)
src/pages/aiConfig/skills/locale/en_US.tssrc/pages/aiConfig/skills/locale/ja_JP.tssrc/pages/aiConfig/skills/locale/ru_RU.tssrc/pages/aiConfig/skills/locale/zh_CN.tssrc/pages/aiConfig/skills/locale/zh_HK.tssrc/pages/aiConfig/skills/pages/List.tsxsrc/pages/aiConfig/skills/pages/SkillDetailPanel.tsxsrc/pages/aiConfig/skills/pages/SkillSidebar.tsxsrc/pages/aiConfig/skills/pages/UploadSkillModal.tsx
…ding
The replace dialog for non-builtin skills now defers submission until the user clicks OK, so they can update only the managing team and visibility without re-selecting an archive. Builtin skills keep the existing pick-to-upload flow. The detail panel also surfaces the managing team and visibility in its meta section.
Summary by CodeRabbit
New Features
Bug Fixes