feat(web): Import Skill modal + fix controller symlink crash#355
Merged
alchemistklk merged 6 commits intomainfrom Mar 23, 2026
Merged
feat(web): Import Skill modal + fix controller symlink crash#355alchemistklk merged 6 commits intomainfrom
alchemistklk merged 6 commits intomainfrom
Conversation
On process restart, lastWrittenContent is null, causing the first write() to always hit disk even if the config file already has identical content. This triggers an unnecessary OpenClaw reload on every controller restart. Read the existing file to seed the in-memory cache on the first write() call, so cold starts skip the write when content matches.
Replace the inline hidden file input + button with a polished modal dialog ported from the design-system. Two tabs: Upload Zip (wired to existing zip import backend) and GitHub Link (placeholder, coming soon). Adds Dialog UI component (Radix-based) and i18n keys for EN/ZH-CN. Also fixes controller startup crash (EINVAL) when copying runtime plugin dirs containing node_modules/.bin symlinks by filtering out symlinks during the recursive copy.
Contributor
Author
Code reviewFound 2 issues:
nexu/apps/controller/src/runtime/openclaw-runtime-plugin-writer.ts Lines 41 to 49 in 189adef
nexu/apps/controller/src/runtime/openclaw-runtime-plugin-writer.ts Lines 28 to 49 in 189adef 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…lter Keep main's more targeted approach (dereference: true + skip .bin dirs) instead of the broader all-symlink filter. Both fix the same EINVAL crash but main's approach is more specific and preserves useful symlinks.
📝 WalkthroughWalkthroughThis PR adds a modal-based skill import flow (ZIP and GitHub tab), a Radix dialog wrapper, i18n entries for import UI (en and zh-CN), state utilities and tests for the modal logic, and a controller test validating plugin-template symlink handling. Changes
Sequence DiagramsequenceDiagram
actor User
participant SkillsPage
participant ImportSkillModal
participant useImportSkill
participant Backend
User->>SkillsPage: Click "Import"
SkillsPage->>ImportSkillModal: Open modal
User->>ImportSkillModal: Select/drag ZIP
ImportSkillModal->>ImportSkillModal: Validate ZIP (getSelectedZipFile)
User->>ImportSkillModal: Click Import
ImportSkillModal->>useImportSkill: mutateAsync(selectedFile)
useImportSkill->>Backend: Upload & process ZIP
alt Success
Backend-->>useImportSkill: Success
useImportSkill-->>ImportSkillModal: resolve
ImportSkillModal->>ImportSkillModal: Show success, schedule auto-close (1200ms)
ImportSkillModal->>SkillsPage: Trigger close
else Error
Backend-->>useImportSkill: Error
useImportSkill-->>ImportSkillModal: reject
ImportSkillModal->>ImportSkillModal: Show error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/controller/tests/openclaw-runtime-plugin-writer.test.ts`:
- Around line 71-97: The test expects non-.bin symlinks to be preserved but
OpenClawRuntimePluginWriter.ensurePlugins() calls cp() with dereference: true
which materializes symlinks; either change the writer to preserve symlinks by
setting dereference: false in the cp() invocation inside
OpenClawRuntimePluginWriter.ensurePlugins(), or update the test to expect a
materialized directory (remove the isSymbolicLink() assertion and assert
directory contents instead) so behavior and test align.
In `@apps/web/src/components/skills/import-skill-modal.tsx`:
- Around line 67-79: When a new file is chosen or dropped, the previous import
mutation error must be cleared; update handleFileChange and handleDrop to call
the import mutation's reset/clear method (e.g., importSkillMutation.reset() or
importSkillMutation.resetError()) immediately after
setSelectedFile(getSelectedZipFile(file)) so any prior error state is dismissed
when a new file is selected/dropped; keep the existing e.target.value = "" reset
in handleFileChange.
- Around line 18-21: The import './import-skill-modal-state' is unresolved
causing TS2307; either add the missing module exporting the helpers or
remove/inline the imports. Fix by creating a module that exports
createAutoCloseController and getSelectedZipFile (matching those exact function
names) or update the import to the correct path where those functions live;
alternatively inline implementations of createAutoCloseController and
getSelectedZipFile directly into import-skill-modal.tsx ensuring their
signatures match existing usage in the component.
- Around line 196-197: Replace the hardcoded fallback string "Import failed"
used with importMutation.error?.message ?? "Import failed" inside the
ImportSkillModal component so it uses the i18n translation key (e.g., call your
translation helper for "skills.importFailed") instead of plain English; also add
the new "skills.importFailed" entry to both locale files (with appropriate
translations) so the fallback is localized across languages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c6c3484-0476-4190-a432-063ce87f2204
📒 Files selected for processing (6)
apps/controller/tests/openclaw-runtime-plugin-writer.test.tsapps/web/src/components/skills/import-skill-modal.tsxapps/web/src/components/ui/dialog.tsxapps/web/src/i18n/locales/en.tsapps/web/src/i18n/locales/zh-CN.tsapps/web/src/pages/skills.tsx
The modal state helpers (getSelectedZipFile, createAutoCloseController) were extracted but not committed, breaking the build with TS2307.
The test expected non-.bin symlinks to be preserved as symlinks, but the implementation uses dereference: true which materializes them into real directories. Update the test to assert directory materialization instead of symlink preservation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/tests/import-skill-modal-state.test.ts (1)
7-17: Consider locking in edge cases for ZIP selection.Coverage is good; adding uppercase extension and nullish-input cases would better pin the intended contract.
✅ Suggested test additions
describe("getSelectedZipFile", () => { it("keeps valid zip files", () => { const file = { name: "skill.zip" }; expect(getSelectedZipFile(file)).toBe(file); }); + it("keeps valid zip files with uppercase extension", () => { + const file = { name: "skill.ZIP" }; + expect(getSelectedZipFile(file)).toBe(file); + }); + + it("returns null for nullish input", () => { + expect(getSelectedZipFile(null)).toBeNull(); + expect(getSelectedZipFile(undefined)).toBeNull(); + }); + it("clears the selection for invalid files", () => { expect(getSelectedZipFile({ name: "skill.txt" })).toBeNull(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/tests/import-skill-modal-state.test.ts` around lines 7 - 17, Add tests to strengthen getSelectedZipFile's contract by asserting it accepts ZIP extensions case-insensitively and handles null/undefined inputs: add a test that passes a file object with name "SKILL.ZIP" (and/or mixed case) and expects the same file returned, and add tests that call getSelectedZipFile(null) and getSelectedZipFile(undefined) (or omit the argument) and expect null; update or add these cases near the existing describe("getSelectedZipFile") block to ensure uppercase extension and nullish-input edge cases are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/tests/import-skill-modal-state.test.ts`:
- Around line 7-17: Add tests to strengthen getSelectedZipFile's contract by
asserting it accepts ZIP extensions case-insensitively and handles
null/undefined inputs: add a test that passes a file object with name
"SKILL.ZIP" (and/or mixed case) and expects the same file returned, and add
tests that call getSelectedZipFile(null) and getSelectedZipFile(undefined) (or
omit the argument) and expect null; update or add these cases near the existing
describe("getSelectedZipFile") block to ensure uppercase extension and
nullish-input edge cases are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4c88cc7-a177-4baa-93da-8609de5d20d1
📒 Files selected for processing (3)
apps/controller/tests/openclaw-runtime-plugin-writer.test.tsapps/web/src/components/skills/import-skill-modal-state.tsapps/web/tests/import-skill-modal-state.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/controller/tests/openclaw-runtime-plugin-writer.test.ts
Contributor
Author
|
/cr |
|
✅ CR topic created in Feishu topic group Refly CR. |
lefarcen
approved these changes
Mar 23, 2026
This was referenced Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #375
Summary
DialogUI component (Radix-based).EINVALcrash whenfs.cp()copies runtime plugin directories containingnode_modules/.binsymlinks. Symlinks are now filtered out during the recursive copy.Test plan
pnpm typecheckpassespnpm lintpassespnpm dev— open Skills page, click Import button, verify modal opens.zipfiles, import workspnpm start— desktop app launches past "Starting local services" screen🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI/UX
Internationalization
Tests