feat(Skills): added action to create skills#1108
Merged
gastoner merged 2 commits intokortex-hub:mainfrom Mar 19, 2026
Merged
Conversation
gastoner
requested review from
MarsKubeX and
benoitf
and removed request for
a team
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-target skill creation: new types for per-folder info and file content, updates SkillManager to register/list folders, discovers and writes skills per target, updates IPC/preload surface, and adds modal UI components and tests for target-aware skill creation and content parsing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as SkillCreate Modal
participant Preload
participant Main as SkillManager
participant FS as FileSystem
User->>UI: open modal / select target
UI->>Preload: ipcInvoke('skill-manager:listSkillFolders')
Preload->>Main: IPC: listSkillFolders
Main-->>Preload: [SkillFolderInfo...]
Preload-->>UI: [SkillFolderInfo...]
User->>UI: provide/import skill content
UI->>Preload: createSkill(options, targetDirectory)
Preload->>Main: IPC: createSkill(options, targetDirectory)
Main->>Main: validateSkillFolder(targetDirectory)
Main->>FS: write SKILL.md to targetDirectory
Main->>Main: discoverSkillsInDirectory(targetDirectory)
Main-->>Preload: SkillInfo
Preload-->>UI: SkillInfo (success)
UI->>User: close modal / show success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/renderer/src/lib/skills/SkillsList.spec.ts (1)
30-30: Usevi.mock(import('...'))syntax for type-safe mocking.Per coding guidelines, use the import syntax to ensure TypeScript validates the module path at compile time.
♻️ Proposed fix
-vi.mock('/@/stores/skills'); +vi.mock(import('/@/stores/skills'));As per coding guidelines: "Use
vi.mock(import('...'))for auto-mocking modules in unit tests; avoid manual mock factories when possible"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/skills/SkillsList.spec.ts` at line 30, The test currently calls vi.mock('/@/stores/skills') which bypasses TypeScript path checking; update the mock to use the import-style form so the module path is type-checked. Replace the invocation of vi.mock in SkillsList.spec.ts (the call referencing '/@/stores/skills') with the import(...) form (i.e., vi.mock(import('...'))) and remove any manual mock factory if present so the auto-mocking via vi.mock(import('...')) is used instead.packages/renderer/src/lib/ui/CardSelector.svelte (1)
38-38: Consider usingoption.titlefor a more descriptivearia-label.The button's
aria-labelusesoption.value(e.g., "a", "b", "kortex", "claude"), which may not be meaningful to screen reader users. Usingoption.titlewould provide better accessibility.♻️ Proposed change
- aria-label={option.value} + aria-label={option.title}Note: This would require updating the tests that query by
aria-labelusing the value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/ui/CardSelector.svelte` at line 38, In CardSelector.svelte change the button's aria-label to use option.title instead of option.value to provide a more descriptive label for screen readers (locate the attribute aria-label={option.value} in the CardSelector component and replace with aria-label={option.title}); after updating, adjust any tests that query by aria-label to expect the option.title strings rather than the short option.value ones.packages/main/src/plugin/skill/skill-manager.spec.ts (1)
37-40: Prefer the repo'svi.mock(import(...))pattern fornode:os.This manual factory drifts from the test style used elsewhere and loses the refactor-safe module-path checking. Auto-mock
node:osand setvi.mocked(homedir).mockReturnValue('/home/test')inbeforeEachinstead.Based on learnings, in the kortex repository tests use
vi.mock(import('module-path'))syntax instead ofvi.mock('module-path')to ensure TypeScript validates the module path at compile time, providing refactoring safety if modules are moved or removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.spec.ts` around lines 37 - 40, Replace the manual factory vi.mock('node:os', ...) with the repo-standard auto-mock pattern using vi.mock(await import('node:os')) so TypeScript validates the module path; then in the test setup (beforeEach) call vi.mocked(homedir).mockReturnValue('/home/test') to stub homedir. Locate the current mock invoking vi.importActual/heterogeneous factory and change it to vi.mock(await import('node:os')) and add the vi.mocked(homedir).mockReturnValue call in beforeEach (reference symbol: homedir, vi.mock, vi.mocked, beforeEach).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 279-286: The code treats Claude's skills directory as managed only
in getBaseDirectoryForTarget but not elsewhere; create a single source of truth
(e.g., a new private method or property like getManagedSkillRoots() or
managedRoots) that returns an array of managed root directories including
this.directories.getSkillsDirectory() and resolve(homedir(), '.claude',
'skills'), then update discoverSkills(), isManagedSkill(), and unregisterSkill()
to use that list instead of hardcoding directories.getSkillsDirectory() so
Claude-created skills are discovered, recognized as managed, and removed on
unregister.
In `@packages/renderer/src/lib/skills/SkillCreate.svelte`:
- Around line 116-123: The drop/file-picker path currently sets selectedFile =
selected and then swallows parse errors by clearing skillContent in the catch,
which leaves the file marked selected but the editor empty; update the flow in
handleDrop()/the file selection handler around window.getSkillFileContent and
prefillFromParsed so that on parse failure you either (A) fetch and use the raw
file text (call a raw-text IPC helper instead of discarding) and pass that into
prefillFromParsed or assign it to skillContent, or (B) revert the selection
(clear selectedFile) and surface a blocking error to the user instead of
silently clearing skillContent; ensure the failure branch does not leave
selectedFile set while skillContent is empty and reference the functions/vars
getSkillFileContent, prefillFromParsed, handleDrop, selectedFile, and
skillContent when making the change.
- Around line 79-83: The prefillFromParsed function assigns user-controlled
frontmatter directly into form state (name, description, skillContent) which can
cause runtime errors if non-strings are provided; update prefillFromParsed to
only copy values when they are strings by guarding assignments with typeof ===
'string' for name and description (and optionally skillContent) so later .trim()
calls and validity checks won't throw. Ensure you reference and update the
prefillFromParsed function and the variables name, description, and skillContent
when adding these guards.
In `@packages/renderer/src/lib/skills/SkillTargetCards.svelte`:
- Line 2: The imported symbol faClaude does not exist in
`@fortawesome/free-brands-svg-icons` and will break the build; remove the import
statement for faClaude and either replace its usage with a valid FontAwesome
icon (e.g., import faRobot or another icon from
`@fortawesome/free-solid-svg-icons`) or provide a custom SVG icon, and then update
all references to faClaude inside the SkillTargetCards.svelte component (replace
occurrences of faClaude in any <FontAwesomeIcon> or icon props with the chosen
valid icon or remove the icon usage entirely).
In `@packages/renderer/src/lib/ui/CardSelector.spec.ts`:
- Around line 80-90: The test fails because CardSelector.svelte's handleClick
returns early when the clicked option equals the current selected value,
preventing deselection; update handleClick in CardSelector.svelte so that when
the clicked option === selected it clears the selection (e.g., emit or set
selected to null/undefined) instead of returning early, and ensure the component
still emits the same change event (or update the selected binding) so the spec's
expectation that both checkboxes become unchecked after clicking the
already-selected option is satisfied; reference the handleClick function and the
selected prop/emit logic to locate and change the behavior.
In `@packages/renderer/src/lib/ui/CardSelector.svelte`:
- Around line 21-24: The current handleClick function prevents deselection by
returning early when selected === value; change its logic in the handleClick
function so it toggles selection instead: if selected === value set selected to
an empty string (deselect), otherwise set selected to the clicked value; update
any dependent state/dispatching code around handleClick/selected to reflect that
deselection is now allowed so the test "should deselect when clicking the
already selected option" passes.
---
Nitpick comments:
In `@packages/main/src/plugin/skill/skill-manager.spec.ts`:
- Around line 37-40: Replace the manual factory vi.mock('node:os', ...) with the
repo-standard auto-mock pattern using vi.mock(await import('node:os')) so
TypeScript validates the module path; then in the test setup (beforeEach) call
vi.mocked(homedir).mockReturnValue('/home/test') to stub homedir. Locate the
current mock invoking vi.importActual/heterogeneous factory and change it to
vi.mock(await import('node:os')) and add the vi.mocked(homedir).mockReturnValue
call in beforeEach (reference symbol: homedir, vi.mock, vi.mocked, beforeEach).
In `@packages/renderer/src/lib/skills/SkillsList.spec.ts`:
- Line 30: The test currently calls vi.mock('/@/stores/skills') which bypasses
TypeScript path checking; update the mock to use the import-style form so the
module path is type-checked. Replace the invocation of vi.mock in
SkillsList.spec.ts (the call referencing '/@/stores/skills') with the
import(...) form (i.e., vi.mock(import('...'))) and remove any manual mock
factory if present so the auto-mocking via vi.mock(import('...')) is used
instead.
In `@packages/renderer/src/lib/ui/CardSelector.svelte`:
- Line 38: In CardSelector.svelte change the button's aria-label to use
option.title instead of option.value to provide a more descriptive label for
screen readers (locate the attribute aria-label={option.value} in the
CardSelector component and replace with aria-label={option.title}); after
updating, adjust any tests that query by aria-label to expect the option.title
strings rather than the short option.value ones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c89082a-872c-4c30-8674-b6ead0ce6900
📒 Files selected for processing (12)
packages/api/src/skill/skill-info.tspackages/main/src/plugin/skill/skill-manager.spec.tspackages/main/src/plugin/skill/skill-manager.tspackages/preload/src/index.tspackages/renderer/src/lib/skills/SkillCreate.spec.tspackages/renderer/src/lib/skills/SkillCreate.sveltepackages/renderer/src/lib/skills/SkillEmptyScreen.sveltepackages/renderer/src/lib/skills/SkillTargetCards.sveltepackages/renderer/src/lib/skills/SkillsList.spec.tspackages/renderer/src/lib/skills/SkillsList.sveltepackages/renderer/src/lib/ui/CardSelector.spec.tspackages/renderer/src/lib/ui/CardSelector.svelte
Comment on lines
+79
to
+83
| function prefillFromParsed(parsed: SkillFileContent): void { | ||
| if (parsed.name) name = parsed.name; | ||
| if (parsed.description) description = parsed.description; | ||
| if (parsed.content) skillContent = parsed.content; | ||
| } |
There was a problem hiding this comment.
Only copy string frontmatter into form state.
Imported frontmatter is user-controlled. If name or description comes back as a non-string value, the later .trim() calls in the validity check / create() path will throw before backend validation runs. Guard these assignments with typeof === 'string'.
🛡️ Suggested guard
function prefillFromParsed(parsed: SkillFileContent): void {
- if (parsed.name) name = parsed.name;
- if (parsed.description) description = parsed.description;
- if (parsed.content) skillContent = parsed.content;
+ if (typeof parsed.name === 'string' && parsed.name) name = parsed.name;
+ if (typeof parsed.description === 'string' && parsed.description) description = parsed.description;
+ if (typeof parsed.content === 'string' && parsed.content) skillContent = parsed.content;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function prefillFromParsed(parsed: SkillFileContent): void { | |
| if (parsed.name) name = parsed.name; | |
| if (parsed.description) description = parsed.description; | |
| if (parsed.content) skillContent = parsed.content; | |
| } | |
| function prefillFromParsed(parsed: SkillFileContent): void { | |
| if (typeof parsed.name === 'string' && parsed.name) name = parsed.name; | |
| if (typeof parsed.description === 'string' && parsed.description) description = parsed.description; | |
| if (typeof parsed.content === 'string' && parsed.content) skillContent = parsed.content; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/skills/SkillCreate.svelte` around lines 79 - 83,
The prefillFromParsed function assigns user-controlled frontmatter directly into
form state (name, description, skillContent) which can cause runtime errors if
non-strings are provided; update prefillFromParsed to only copy values when they
are strings by guarding assignments with typeof === 'string' for name and
description (and optionally skillContent) so later .trim() calls and validity
checks won't throw. Ensure you reference and update the prefillFromParsed
function and the variables name, description, and skillContent when adding these
guards.
Comment on lines
+80
to
+90
| test('should deselect when clicking the already selected option', async () => { | ||
| render(CardSelector, { options, selected: 'a' }); | ||
|
|
||
| await fireEvent.click(screen.getByLabelText('a')); | ||
|
|
||
| const checkboxA = screen.getByRole('checkbox', { name: 'Option A' }); | ||
| const checkboxB = screen.getByRole('checkbox', { name: 'Option B' }); | ||
|
|
||
| expect(checkboxA).not.toBeChecked(); | ||
| expect(checkboxB).not.toBeChecked(); | ||
| }); |
There was a problem hiding this comment.
This test will fail due to implementation mismatch.
As noted in the CardSelector.svelte review, the handleClick function returns early when clicking the already-selected option, preventing deselection. This test expects clicking option "a" (already selected) to result in both checkboxes being unchecked, which won't happen with the current implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/ui/CardSelector.spec.ts` around lines 80 - 90, The
test fails because CardSelector.svelte's handleClick returns early when the
clicked option equals the current selected value, preventing deselection; update
handleClick in CardSelector.svelte so that when the clicked option === selected
it clears the selection (e.g., emit or set selected to null/undefined) instead
of returning early, and ensure the component still emits the same change event
(or update the selected binding) so the spec's expectation that both checkboxes
become unchecked after clicking the already-selected option is satisfied;
reference the handleClick function and the selected prop/emit logic to locate
and change the behavior.
efca11b to
01a8d04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/renderer/src/lib/skills/SkillCreate.svelte (2)
79-83:⚠️ Potential issue | 🟠 MajorGuard and normalize parsed values before writing form state.
This currently trusts parsed frontmatter shape and only updates truthy fields, which can both cause runtime
.trim()failures and keep stale values from previous imports.💡 Suggested fix
function prefillFromParsed(parsed: SkillFileContent): void { - if (parsed.name) name = parsed.name; - if (parsed.description) description = parsed.description; - if (parsed.content) skillContent = parsed.content; + name = typeof parsed.name === 'string' ? parsed.name : ''; + description = typeof parsed.description === 'string' ? parsed.description : ''; + skillContent = typeof parsed.content === 'string' ? parsed.content : ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/skills/SkillCreate.svelte` around lines 79 - 83, The prefillFromParsed function trusts parsed fields and only assigns truthy values, which can leave stale state and cause .trim() on non-strings; update it to check for the presence of each property (e.g., use 'name' in parsed / hasOwnProperty) and always assign a normalized string value to the component state variables (name, description, skillContent), converting non-string or null/undefined to '' and safely calling .trim() only on strings so imports clear previous values and avoid runtime errors; reference the prefillFromParsed function and the SkillFileContent shape when making these guards and normalizations.
116-123:⚠️ Potential issue | 🟠 MajorDon’t keep a selected file when parse/import fails.
The catch path leaves
selectedFileset while content is emptied, which traps the form in a broken state for that selection.💡 Suggested fix
selectedFile = selected; try { const parsed = await window.getSkillFileContent(selected); prefillFromParsed(parsed); } catch { - skillContent = ''; + selectedFile = ''; + error = 'Failed to parse selected SKILL.md file.'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/skills/SkillCreate.svelte` around lines 116 - 123, The code sets selectedFile = selected before attempting to parse via window.getSkillFileContent, so on parse/import failure the UI remains stuck with a selected file while skillContent is cleared; move the assignment or clear selectedFile on error: either assign selectedFile only after await window.getSkillFileContent(...) and successful prefillFromParsed(parsed), or in the catch block also reset selectedFile (e.g., to null/''), referencing selectedFile, selected, window.getSkillFileContent, prefillFromParsed, and skillContent to ensure the form state is consistent when parsing fails.packages/main/src/plugin/skill/skill-manager.ts (1)
279-287:⚠️ Potential issue | 🟠 MajorCentralize managed roots; Claude skills are still treated as external.
Adding target-aware creation here is incomplete while
isManagedSkill()anddiscoverSkills()still assume only the Kortex root, which affects unregister/delete and config persistence semantics.💡 Suggested direction
+ private getManagedSkillRoots(): string[] { + return [this.directories.getSkillsDirectory(), resolve(homedir(), '.claude', 'skills')]; + } + private getBaseDirectoryForTarget(target: SkillTarget): string { switch (target) { case 'claude': return resolve(homedir(), '.claude', 'skills'); case 'kortex': default: return this.directories.getSkillsDirectory(); } } ... private isManagedSkill(skill: SkillInfo): boolean { - const skillsDir = this.directories.getSkillsDirectory(); - return skill.path.startsWith(skillsDir); + return this.getManagedSkillRoots().some(root => skill.path.startsWith(root)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 279 - 287, getBaseDirectoryForTarget added target-aware paths for Claude, but isManagedSkill() and discoverSkills() still only consider the Kortex root causing Claude skills to be treated as external; update those methods to consult the same target-aware root logic. Modify isManagedSkill(skillId, target?) and discoverSkills(target?) (or overload existing signatures) to call getBaseDirectoryForTarget(target: SkillTarget) when determining managed roots and when scanning/reading persisted configs, and ensure unregister/delete operations use that same target-aware directory so persistence and deletion semantics are consistent for all SkillTarget values (reference functions: getBaseDirectoryForTarget, isManagedSkill, discoverSkills, and any unregister/delete handlers).
🧹 Nitpick comments (1)
packages/main/src/plugin/skill/skill-manager.spec.ts (1)
37-40: Use project-standard auto-mock style fornode:os.Please switch this to
vi.mock(import('node:os'))and stubhomedirviavi.mocked(...)in setup to match repo test conventions.As per coding guidelines `**/*.spec.ts`: Use `vi.mock(import('...'))` for auto-mocking modules in tests; avoid manual mock factories (`vi.mock('...', () => ({...}))`) when possible.💡 Suggested refactor
+import { homedir } from 'node:os'; ... -vi.mock('node:os', async () => ({ - ...(await vi.importActual('node:os')), - homedir: (): string => '/home/test', -})); +vi.mock(import('node:os'));beforeEach(() => { vi.resetAllMocks(); + vi.mocked(homedir).mockReturnValue('/home/test'); console.warn = vi.fn(); getMock.mockReturnValue([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.spec.ts` around lines 37 - 40, Replace the manual mock factory for node:os with the repo-standard auto-mock pattern: call vi.mock(import('node:os')) at top of the test file and then, in the test setup/beforeEach, use vi.mocked(await import('node:os')) (or vi.mocked(osModule)) to stub the homedir method to return '/home/test'; update any references to the original mock factory and ensure the homedir stub is restored/reset between tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 344-349: The getSkillFileContent function returns metadata.name
and metadata.description as-is but extractFrontmatter may produce non-string
values; update getSkillFileContent to coerce/normalize those fields into strings
(e.g., if value is undefined/null use empty string, otherwise call
String(value)) before returning the SkillFileContent so renderer assumptions
hold; reference getSkillFileContent, extractFrontmatter, and the
SkillFileContent return object when applying the normalization.
---
Duplicate comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 279-287: getBaseDirectoryForTarget added target-aware paths for
Claude, but isManagedSkill() and discoverSkills() still only consider the Kortex
root causing Claude skills to be treated as external; update those methods to
consult the same target-aware root logic. Modify isManagedSkill(skillId,
target?) and discoverSkills(target?) (or overload existing signatures) to call
getBaseDirectoryForTarget(target: SkillTarget) when determining managed roots
and when scanning/reading persisted configs, and ensure unregister/delete
operations use that same target-aware directory so persistence and deletion
semantics are consistent for all SkillTarget values (reference functions:
getBaseDirectoryForTarget, isManagedSkill, discoverSkills, and any
unregister/delete handlers).
In `@packages/renderer/src/lib/skills/SkillCreate.svelte`:
- Around line 79-83: The prefillFromParsed function trusts parsed fields and
only assigns truthy values, which can leave stale state and cause .trim() on
non-strings; update it to check for the presence of each property (e.g., use
'name' in parsed / hasOwnProperty) and always assign a normalized string value
to the component state variables (name, description, skillContent), converting
non-string or null/undefined to '' and safely calling .trim() only on strings so
imports clear previous values and avoid runtime errors; reference the
prefillFromParsed function and the SkillFileContent shape when making these
guards and normalizations.
- Around line 116-123: The code sets selectedFile = selected before attempting
to parse via window.getSkillFileContent, so on parse/import failure the UI
remains stuck with a selected file while skillContent is cleared; move the
assignment or clear selectedFile on error: either assign selectedFile only after
await window.getSkillFileContent(...) and successful prefillFromParsed(parsed),
or in the catch block also reset selectedFile (e.g., to null/''), referencing
selectedFile, selected, window.getSkillFileContent, prefillFromParsed, and
skillContent to ensure the form state is consistent when parsing fails.
---
Nitpick comments:
In `@packages/main/src/plugin/skill/skill-manager.spec.ts`:
- Around line 37-40: Replace the manual mock factory for node:os with the
repo-standard auto-mock pattern: call vi.mock(import('node:os')) at top of the
test file and then, in the test setup/beforeEach, use vi.mocked(await
import('node:os')) (or vi.mocked(osModule)) to stub the homedir method to return
'/home/test'; update any references to the original mock factory and ensure the
homedir stub is restored/reset between tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91a26b55-18da-4f1b-bdc7-465853591d1a
📒 Files selected for processing (12)
packages/api/src/skill/skill-info.tspackages/main/src/plugin/skill/skill-manager.spec.tspackages/main/src/plugin/skill/skill-manager.tspackages/preload/src/index.tspackages/renderer/src/lib/skills/SkillCreate.spec.tspackages/renderer/src/lib/skills/SkillCreate.sveltepackages/renderer/src/lib/skills/SkillEmptyScreen.sveltepackages/renderer/src/lib/skills/SkillTargetCards.sveltepackages/renderer/src/lib/skills/SkillsList.spec.tspackages/renderer/src/lib/skills/SkillsList.sveltepackages/renderer/src/lib/ui/CardSelector.spec.tspackages/renderer/src/lib/ui/CardSelector.svelte
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/renderer/src/lib/skills/SkillsList.svelte
- packages/renderer/src/lib/ui/CardSelector.svelte
- packages/renderer/src/lib/skills/SkillTargetCards.svelte
- packages/renderer/src/lib/skills/SkillCreate.spec.ts
- packages/api/src/skill/skill-info.ts
- packages/renderer/src/lib/ui/CardSelector.spec.ts
Comment on lines
+344
to
+349
| async getSkillFileContent(filePath: string): Promise<SkillFileContent> { | ||
| const rawContent = (await readFile(filePath, 'utf-8')).trimStart(); | ||
| const metadata = this.extractFrontmatter(rawContent, filePath); | ||
| const body = this.stripFrontmatter(rawContent); | ||
| return { name: metadata.name, description: metadata.description, content: body }; | ||
| } |
There was a problem hiding this comment.
Normalize metadata types before returning SkillFileContent.
extractFrontmatter() can yield non-string values at runtime, and this method currently forwards them directly. That can break renderer assumptions that name/description are strings.
💡 Suggested fix
async getSkillFileContent(filePath: string): Promise<SkillFileContent> {
const rawContent = (await readFile(filePath, 'utf-8')).trimStart();
const metadata = this.extractFrontmatter(rawContent, filePath);
const body = this.stripFrontmatter(rawContent);
- return { name: metadata.name, description: metadata.description, content: body };
+ return {
+ name: typeof metadata.name === 'string' ? metadata.name : '',
+ description: typeof metadata.description === 'string' ? metadata.description : '',
+ content: body,
+ };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 344 - 349, The
getSkillFileContent function returns metadata.name and metadata.description
as-is but extractFrontmatter may produce non-string values; update
getSkillFileContent to coerce/normalize those fields into strings (e.g., if
value is undefined/null use empty string, otherwise call String(value)) before
returning the SkillFileContent so renderer assumptions hold; reference
getSkillFileContent, extractFrontmatter, and the SkillFileContent return object
when applying the normalization.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
gastoner
force-pushed
the
create_skill_action
branch
from
01a8d04 to
5134a94
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/main/src/plugin/skill/skill-manager.ts (2)
344-349:⚠️ Potential issue | 🟠 MajorNormalize frontmatter fields before returning
SkillFileContent.Line 348 returns
metadata.name/metadata.descriptionwithout type coercion. This can violate theSkillFileContentstring contract and break renderer prefill assumptions.💡 Suggested fix
async getSkillFileContent(filePath: string): Promise<SkillFileContent> { const rawContent = (await readFile(filePath, 'utf-8')).trimStart(); const metadata = this.extractFrontmatter(rawContent, filePath); const body = this.stripFrontmatter(rawContent); - return { name: metadata.name, description: metadata.description, content: body }; + return { + name: metadata.name == null ? '' : String(metadata.name), + description: metadata.description == null ? '' : String(metadata.description), + content: body, + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 344 - 349, getSkillFileContent returns metadata.name/metadata.description directly which may be non-string and violate the SkillFileContent contract; change getSkillFileContent to normalize both fields before returning by coercing to strings and trimming/using empty-string fallbacks (use extractFrontmatter result from extractFrontmatter and stripFrontmatter to build the body, then compute name = String(metadata.name ?? '').trim() and description = String(metadata.description ?? '').trim() or equivalent) so the returned SkillFileContent always has string name and description.
279-287:⚠️ Potential issue | 🟠 MajorTreat Claude skills root as managed globally, not only at create time.
Line 282 writes Claude skills to
~/.claude/skills, but managed-skill checks still only use the Kortex root (Line 365 onward). This causes Claude-created skills to be persisted as external paths and prevents folder deletion onunregisterSkill().💡 Suggested refactor
+import { join, resolve, sep } from 'node:path'; ... + private getManagedSkillRoots(): string[] { + return [this.directories.getSkillsDirectory(), resolve(homedir(), '.claude', 'skills')]; + } + private getBaseDirectoryForTarget(target: SkillTarget): string { switch (target) { case 'claude': - return resolve(homedir(), '.claude', 'skills'); + return this.getManagedSkillRoots()[1]; case 'kortex': default: - return this.directories.getSkillsDirectory(); + return this.getManagedSkillRoots()[0]; } } ... private isManagedSkill(skill: SkillInfo): boolean { - const skillsDir = this.directories.getSkillsDirectory(); - return skill.path.startsWith(skillsDir); + const resolvedSkillPath = resolve(skill.path); + return this.getManagedSkillRoots().some(root => { + const resolvedRoot = resolve(root); + return resolvedSkillPath === resolvedRoot || resolvedSkillPath.startsWith(`${resolvedRoot}${sep}`); + }); } ... - const skillsDir = this.directories.getSkillsDirectory(); - if (existsSync(skillsDir)) { - try { - const entries = await readdir(skillsDir, { withFileTypes: true }); - for (const entry of entries) { - if (entry.isDirectory()) { - folderPaths.push(join(skillsDir, entry.name)); - } - } - } catch { - // ignore read errors on the managed directory - } - } + for (const skillsDir of this.getManagedSkillRoots()) { + if (!existsSync(skillsDir)) continue; + try { + const entries = await readdir(skillsDir, { withFileTypes: true }); + for (const entry of entries) { + if (entry.isDirectory()) folderPaths.push(join(skillsDir, entry.name)); + } + } catch { + // ignore read errors on managed directories + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 279 - 287, The Claude skills base (~/.claude/skills) is being treated as an external path while managed-skill logic still only uses this.directories.getSkillsDirectory(), so Claude-created skills are persisted as external and won't be deleted on unregister. Update all managed-skill checks and any unregister/delete logic (e.g., where you currently call this.directories.getSkillsDirectory()) to use the per-target base directory helper getBaseDirectoryForTarget(target) instead of the hardcoded Kortex root; ensure unregisterSkill and any "isManaged" or path containment checks compute the base with getBaseDirectoryForTarget(skill.target) so Claude skills are recognized as managed and their folders can be removed.
🧹 Nitpick comments (3)
packages/renderer/src/lib/skills/SkillsList.spec.ts (1)
30-30: Consider usingvi.mock(import(...))for auto-mocking.As per coding guidelines, prefer
vi.mock(import('...'))syntax for auto-mocking modules in tests, which provides TypeScript validation of the module path at compile time.-vi.mock('/@/stores/skills'); +vi.mock(import('/@/stores/skills'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/skills/SkillsList.spec.ts` at line 30, Replace the plain string mock call with an import-based mock to enable TypeScript validation; specifically update the vi.mock invocation in SkillsList.spec.ts (the line calling vi.mock('/@/stores/skills')) to use vi.mock(import('/@/stores/skills')) so the module path is checked at compile time and the module is auto-mocked.packages/renderer/src/lib/skills/SkillCreate.spec.ts (1)
197-201: Consider usingwaitForfrom@testing-library/sveltefor consistency.The test uses
vi.waitFor(), butwaitForfrom@testing-library/svelteis already available (it was imported on line 21 but not destructured). Using the testing-library version would be more consistent with the rest of the test patterns.-import { fireEvent, render, screen } from '@testing-library/svelte'; +import { fireEvent, render, screen, waitFor } from '@testing-library/svelte'; ... - await vi.waitFor(() => { + await waitFor(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/skills/SkillCreate.spec.ts` around lines 197 - 201, Replace the usage of vi.waitFor in the test assertion block with the testing-library waitFor: update the import from '@testing-library/svelte' to destructure waitFor (it is already imported but not destructured) and change the call in the test (the async block containing expect(screen.getByLabelText('Skill name')...), currently using vi.waitFor) to use waitFor(...) so the file uses the testing-library's waitFor consistently with other tests.packages/main/src/plugin/skill/skill-manager.spec.ts (1)
37-40: Use Vitest auto-mock import form fornode:osto align with repo conventions.Line 37-40 uses a manual factory mock pattern; the repo test guidelines prefer
vi.mock(import('...'))withvi.mocked(...)for mock setup, providing better type safety and consistency across the codebase.♻️ Suggested change
import { existsSync } from 'node:fs'; import { mkdir, readdir, readFile, rm, writeFile } from 'node:fs/promises'; +import { homedir } from 'node:os'; import { join, resolve } from 'node:path'; import { afterEach, beforeEach, expect, test, vi } from 'vitest'; import type { IPCHandle } from '/@/plugin/api.js'; import type { Directories } from '/@/plugin/directories.js'; import type { ApiSenderType } from '/@api/api-sender/api-sender-type.js'; import type { IConfigurationRegistry } from '/@api/configuration/models.js'; import { SKILL_ENABLED, SKILL_FILE_NAME, SKILL_REGISTERED, type SkillInfo } from '/@api/skill/skill-info.js'; import { SkillManager } from './skill-manager.js'; const SKILLS_DIR = resolve('/test/skills'); vi.mock('node:fs'); vi.mock('node:fs/promises'); -vi.mock('node:os', async () => ({ - ...(await vi.importActual('node:os')), - homedir: (): string => '/home/test', -})); +vi.mock(import('node:os')); const updateMock = vi.fn().mockResolvedValue(undefined); ... beforeEach(() => { vi.resetAllMocks(); + vi.mocked(homedir).mockReturnValue('/home/test'); console.warn = vi.fn(); getMock.mockReturnValue([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.spec.ts` around lines 37 - 40, Replace the manual factory mock of node:os with the repo-preferred auto-mock import pattern: call vi.mock(await import('node:os')) (or vi.mock(import('node:os')) in top-level) and then use vi.mocked on the imported module to override homedir; specifically, remove the factory vi.mock(...) block and instead import node:os in the test, call vi.mocked(os).homedir.mockImplementation(() => '/home/test') (or equivalent) so homedir is mocked with proper typing and consistency with the rest of the test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/renderer/src/lib/skills/SkillCreate.svelte`:
- Around line 66-68: parseSkillContent currently calls load(yamlBlock) which can
throw on malformed YAML, causing an unhandled exception to bubble up to
handleDrop; wrap the call to load(yamlBlock) in a try-catch inside
parseSkillContent (the function that uses trimmed, DELIMITER, endIndex and
constructs yamlBlock) and on error return undefined (or otherwise handle the
parse error) and optionally log or attach the error for debugging so malformed
frontmatter does not crash handleDrop.
---
Duplicate comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 344-349: getSkillFileContent returns
metadata.name/metadata.description directly which may be non-string and violate
the SkillFileContent contract; change getSkillFileContent to normalize both
fields before returning by coercing to strings and trimming/using empty-string
fallbacks (use extractFrontmatter result from extractFrontmatter and
stripFrontmatter to build the body, then compute name = String(metadata.name ??
'').trim() and description = String(metadata.description ?? '').trim() or
equivalent) so the returned SkillFileContent always has string name and
description.
- Around line 279-287: The Claude skills base (~/.claude/skills) is being
treated as an external path while managed-skill logic still only uses
this.directories.getSkillsDirectory(), so Claude-created skills are persisted as
external and won't be deleted on unregister. Update all managed-skill checks and
any unregister/delete logic (e.g., where you currently call
this.directories.getSkillsDirectory()) to use the per-target base directory
helper getBaseDirectoryForTarget(target) instead of the hardcoded Kortex root;
ensure unregisterSkill and any "isManaged" or path containment checks compute
the base with getBaseDirectoryForTarget(skill.target) so Claude skills are
recognized as managed and their folders can be removed.
---
Nitpick comments:
In `@packages/main/src/plugin/skill/skill-manager.spec.ts`:
- Around line 37-40: Replace the manual factory mock of node:os with the
repo-preferred auto-mock import pattern: call vi.mock(await import('node:os'))
(or vi.mock(import('node:os')) in top-level) and then use vi.mocked on the
imported module to override homedir; specifically, remove the factory
vi.mock(...) block and instead import node:os in the test, call
vi.mocked(os).homedir.mockImplementation(() => '/home/test') (or equivalent) so
homedir is mocked with proper typing and consistency with the rest of the test
suite.
In `@packages/renderer/src/lib/skills/SkillCreate.spec.ts`:
- Around line 197-201: Replace the usage of vi.waitFor in the test assertion
block with the testing-library waitFor: update the import from
'@testing-library/svelte' to destructure waitFor (it is already imported but not
destructured) and change the call in the test (the async block containing
expect(screen.getByLabelText('Skill name')...), currently using vi.waitFor) to
use waitFor(...) so the file uses the testing-library's waitFor consistently
with other tests.
In `@packages/renderer/src/lib/skills/SkillsList.spec.ts`:
- Line 30: Replace the plain string mock call with an import-based mock to
enable TypeScript validation; specifically update the vi.mock invocation in
SkillsList.spec.ts (the line calling vi.mock('/@/stores/skills')) to use
vi.mock(import('/@/stores/skills')) so the module path is checked at compile
time and the module is auto-mocked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55be911d-5a15-44c7-8e08-38f7c3774968
📒 Files selected for processing (12)
packages/api/src/skill/skill-info.tspackages/main/src/plugin/skill/skill-manager.spec.tspackages/main/src/plugin/skill/skill-manager.tspackages/preload/src/index.tspackages/renderer/src/lib/skills/SkillCreate.spec.tspackages/renderer/src/lib/skills/SkillCreate.sveltepackages/renderer/src/lib/skills/SkillEmptyScreen.sveltepackages/renderer/src/lib/skills/SkillTargetCards.sveltepackages/renderer/src/lib/skills/SkillsList.spec.tspackages/renderer/src/lib/skills/SkillsList.sveltepackages/renderer/src/lib/ui/CardSelector.spec.tspackages/renderer/src/lib/ui/CardSelector.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/renderer/src/lib/skills/SkillTargetCards.svelte
Comment on lines
+66
to
+68
| const yamlBlock = trimmed.slice(DELIMITER.length + 1, endIndex); | ||
| const parsed = load(yamlBlock); | ||
| if (!parsed || typeof parsed !== 'object') return undefined; |
There was a problem hiding this comment.
Wrap YAML parsing in try-catch to handle malformed frontmatter.
The load() function from js-yaml can throw on invalid YAML syntax. While parseSkillContent returns undefined for missing delimiters, malformed YAML within valid delimiters will cause an unhandled exception that propagates to handleDrop.
🛡️ Suggested fix
const yamlBlock = trimmed.slice(DELIMITER.length + 1, endIndex);
- const parsed = load(yamlBlock);
- if (!parsed || typeof parsed !== 'object') return undefined;
+ let parsed: unknown;
+ try {
+ parsed = load(yamlBlock);
+ } catch {
+ return undefined;
+ }
+ if (!parsed || typeof parsed !== 'object') return undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const yamlBlock = trimmed.slice(DELIMITER.length + 1, endIndex); | |
| const parsed = load(yamlBlock); | |
| if (!parsed || typeof parsed !== 'object') return undefined; | |
| const yamlBlock = trimmed.slice(DELIMITER.length + 1, endIndex); | |
| let parsed: unknown; | |
| try { | |
| parsed = load(yamlBlock); | |
| } catch { | |
| return undefined; | |
| } | |
| if (!parsed || typeof parsed !== 'object') return undefined; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/skills/SkillCreate.svelte` around lines 66 - 68,
parseSkillContent currently calls load(yamlBlock) which can throw on malformed
YAML, causing an unhandled exception to bubble up to handleDrop; wrap the call
to load(yamlBlock) in a try-catch inside parseSkillContent (the function that
uses trimmed, DELIMITER, endIndex and constructs yamlBlock) and on error return
undefined (or otherwise handle the parse error) and optionally log or attach the
error for debugging so malformed frontmatter does not crash handleDrop.
MarsKubeX
reviewed
Mar 17, 2026
Contributor
MarsKubeX
left a comment
MarsKubeX
left a comment
There was a problem hiding this comment.
In general LGTM. I have a few questions.
- After creating a new claude skill and deleting it, I can see that the folder and .md is still under
~/.claude/skills. Is this expected? - Should be a target column in the skills list view to differentiate them?
Contributor
Author
|
benoitf
reviewed
Mar 17, 2026
Contributor
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
hello, do we want to tie the core to claude ?
AFAIK claude extension would bring the logic/storage to store in claude
and let say a cursor extension would bring capability to store in cursor
(agent extension would provide way to store it in the common folder ?)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/main/src/plugin/skill/skill-manager.ts (1)
352-357:⚠️ Potential issue | 🟡 MinorNormalize metadata types before returning
SkillFileContent.
extractFrontmatter()usesjs-yaml'sload()which can yield non-string values at runtime (e.g., numbers, booleans, or null). UnlikeparseSkillFile, this method doesn't callvalidateMetadata, so the returnedname/descriptioncould break renderer assumptions.🛡️ Proposed fix to add type coercion
async getSkillFileContent(filePath: string): Promise<SkillFileContent> { const rawContent = (await readFile(filePath, 'utf-8')).trimStart(); const metadata = this.extractFrontmatter(rawContent, filePath); const body = this.stripFrontmatter(rawContent); - return { name: metadata.name, description: metadata.description, content: body }; + return { + name: typeof metadata.name === 'string' ? metadata.name : String(metadata.name ?? ''), + description: typeof metadata.description === 'string' ? metadata.description : String(metadata.description ?? ''), + content: body, + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 352 - 357, The getSkillFileContent function may return non-string metadata because extractFrontmatter uses js-yaml.load; update getSkillFileContent to normalize/validate metadata the same way parseSkillFile does: coerce metadata.name and metadata.description to strings (e.g., String(metadata.name ?? '')) or call validateMetadata on the extracted metadata before using it, and then return { name, description, content } using those normalized values; reference extractFrontmatter, validateMetadata, parseSkillFile, stripFrontmatter and ensure the renderer-safe string types are returned.
🧹 Nitpick comments (3)
packages/renderer/src/lib/ui/CardSelector.spec.ts (2)
112-123: Consider adding guards for array index access.The non-null assertions (
buttons[1]!,buttons[0]!) assume the DOM structure has at least 2 buttons. If the rendering logic changes, this could cause unclear test failures.🔧 Safer approach with explicit assertions
test('Expect radio indicator filled for selected option', () => { const { container } = render(CardSelector, { options, selected: 'b' }); const buttons = container.querySelectorAll('button'); - const selectedButton = buttons[1]!; + expect(buttons.length).toBeGreaterThanOrEqual(2); + const selectedButton = buttons[1]; const radioFill = selectedButton.querySelector('.w-2.h-2.rounded-full'); expect(radioFill).toBeInTheDocument(); - const unselectedButton = buttons[0]!; + const unselectedButton = buttons[0]; const noRadioFill = unselectedButton.querySelector('.w-2.h-2.rounded-full'); expect(noRadioFill).not.toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/ui/CardSelector.spec.ts` around lines 112 - 123, The test "Expect radio indicator filled for selected option" uses non-null assertions buttons[1]! and buttons[0]! which can cause obscure failures if rendering changes; replace those with explicit guards by asserting the rendered buttons collection length (e.g., expect(buttons.length).toBeGreaterThanOrEqual(2) or use getAllByRole('button') and expect the length) before indexing, then use the guarded elements (selectedButton, unselectedButton) to query for the radio fill; update any variable usages (buttons, selectedButton, unselectedButton, render(CardSelector, ...)) accordingly so the test fails clearly when the DOM does not contain the expected number of buttons.
33-35:vi.resetAllMocks()is called but no mocks exist in this test file.The
beforeEachfollows the coding guideline pattern, but since there are no mocks defined in this file, the call is currently a no-op. This is harmless and could be useful if mocks are added later, so consider this a minor observation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/ui/CardSelector.spec.ts` around lines 33 - 35, The test file contains a no-op call to vi.resetAllMocks() inside the beforeEach hook; either remove the beforeEach block or make intent explicit: delete the beforeEach(() => { vi.resetAllMocks(); }); if you want to keep tests minimal, or keep it but add a short comment above beforeEach stating it's intentionally present to reset future mocks (referencing the beforeEach hook and vi.resetAllMocks) so future additions don't require reintroducing it.packages/main/src/plugin/skill/skill-manager.spec.ts (1)
700-700: Minor: Use a regular string instead of a template literal.Biome flags this as an unnecessary template literal since no interpolation is used. However, template literals are often preferred for multiline strings for readability, so this is a minor style consideration.
🔧 Optional fix
- const EXTRA_SKILL_MD = `---\nname: extra-skill\ndescription: An extra skill\n---\n# Extra\n`; + const EXTRA_SKILL_MD = '---\nname: extra-skill\ndescription: An extra skill\n---\n# Extra\n';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.spec.ts` at line 700, The constant EXTRA_SKILL_MD is defined using a template literal with no interpolation; replace the template literal with a plain string (using escaped newlines or concatenation) so it is a regular string literal instead of a backtick template literal; update the declaration of EXTRA_SKILL_MD in the test file (the constant named EXTRA_SKILL_MD) to use a normal quoted string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 352-357: The getSkillFileContent function may return non-string
metadata because extractFrontmatter uses js-yaml.load; update
getSkillFileContent to normalize/validate metadata the same way parseSkillFile
does: coerce metadata.name and metadata.description to strings (e.g.,
String(metadata.name ?? '')) or call validateMetadata on the extracted metadata
before using it, and then return { name, description, content } using those
normalized values; reference extractFrontmatter, validateMetadata,
parseSkillFile, stripFrontmatter and ensure the renderer-safe string types are
returned.
---
Nitpick comments:
In `@packages/main/src/plugin/skill/skill-manager.spec.ts`:
- Line 700: The constant EXTRA_SKILL_MD is defined using a template literal with
no interpolation; replace the template literal with a plain string (using
escaped newlines or concatenation) so it is a regular string literal instead of
a backtick template literal; update the declaration of EXTRA_SKILL_MD in the
test file (the constant named EXTRA_SKILL_MD) to use a normal quoted string.
In `@packages/renderer/src/lib/ui/CardSelector.spec.ts`:
- Around line 112-123: The test "Expect radio indicator filled for selected
option" uses non-null assertions buttons[1]! and buttons[0]! which can cause
obscure failures if rendering changes; replace those with explicit guards by
asserting the rendered buttons collection length (e.g.,
expect(buttons.length).toBeGreaterThanOrEqual(2) or use getAllByRole('button')
and expect the length) before indexing, then use the guarded elements
(selectedButton, unselectedButton) to query for the radio fill; update any
variable usages (buttons, selectedButton, unselectedButton, render(CardSelector,
...)) accordingly so the test fails clearly when the DOM does not contain the
expected number of buttons.
- Around line 33-35: The test file contains a no-op call to vi.resetAllMocks()
inside the beforeEach hook; either remove the beforeEach block or make intent
explicit: delete the beforeEach(() => { vi.resetAllMocks(); }); if you want to
keep tests minimal, or keep it but add a short comment above beforeEach stating
it's intentionally present to reset future mocks (referencing the beforeEach
hook and vi.resetAllMocks) so future additions don't require reintroducing it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21eb5601-4c00-423d-ab60-7839d5a423f6
📒 Files selected for processing (10)
packages/api/src/skill/skill-info.tspackages/main/src/plugin/skill/skill-manager.spec.tspackages/main/src/plugin/skill/skill-manager.tspackages/preload/src/index.tspackages/renderer/src/lib/skills/SkillCreate.spec.tspackages/renderer/src/lib/skills/SkillCreate.sveltepackages/renderer/src/lib/skills/SkillFolderCards.sveltepackages/renderer/src/lib/skills/SkillsList.spec.tspackages/renderer/src/lib/ui/CardSelector.spec.tspackages/renderer/src/lib/ui/CardSelector.svelte
✅ Files skipped from review due to trivial changes (1)
- packages/renderer/src/lib/skills/SkillFolderCards.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/renderer/src/lib/skills/SkillCreate.svelte
- packages/renderer/src/lib/skills/SkillCreate.spec.ts
gastoner
force-pushed
the
create_skill_action
branch
from
f3f873c to
a33774a
Compare
Contributor
Author
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/renderer/src/lib/skills/SkillCreate.svelte (2)
81-85:⚠️ Potential issue | 🟠 MajorGuard parsed values before assigning form state.
name,description, andskillContentare treated as strings downstream. Assigning untyped parsed values here can trigger runtime failures during validation/creation.🛡️ Proposed fix
function prefillFromParsed(parsed: SkillFileContent): void { - if (parsed.name) name = parsed.name; - if (parsed.description) description = parsed.description; - if (parsed.content) skillContent = parsed.content; + if (typeof parsed.name === 'string' && parsed.name) name = parsed.name; + if (typeof parsed.description === 'string' && parsed.description) description = parsed.description; + if (typeof parsed.content === 'string' && parsed.content) skillContent = parsed.content; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/skills/SkillCreate.svelte` around lines 81 - 85, prefillFromParsed assigns unvalidated parsed values directly to form state (name, description, skillContent); guard these by checking the type/shape before assignment (e.g., ensure parsed.name and parsed.description are strings and parsed.content is a string or convert/default to ""), so update the prefillFromParsed function to only assign when typeof parsed.name === "string" (and similarly for description and content) or otherwise set a safe default to prevent runtime validation errors in downstream code that expects strings.
118-125:⚠️ Potential issue | 🟡 MinorAvoid leaving
selectedFileset when parsing fails.This failure path keeps a file selected while clearing content, which leaves the dialog in an inconsistent state.
💡 Proposed fix
- selectedFile = selected; - try { const parsed = await window.getSkillFileContent(selected); + selectedFile = selected; prefillFromParsed(parsed); } catch { - skillContent = ''; + selectedFile = ''; + error = 'Failed to parse selected SKILL.md file.'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/skills/SkillCreate.svelte` around lines 118 - 125, The catch block leaves selectedFile set while clearing skillContent, causing inconsistent dialog state; update the error path in the try/catch around window.getSkillFileContent(selected) so that when the parse fails you also reset selectedFile (and any related state set by prefillFromParsed) — e.g. set selectedFile to null/empty and clear any parsed-derived fields alongside skillContent so the dialog reflects no selection when parsing fails.packages/main/src/plugin/skill/skill-manager.ts (1)
352-357:⚠️ Potential issue | 🟠 MajorNormalize
getSkillFileContentmetadata fields before returning.This path currently forwards metadata fields as-is. Non-string values can propagate to renderer form state and break later string operations.
🛡️ Proposed fix
async getSkillFileContent(filePath: string): Promise<SkillFileContent> { const rawContent = (await readFile(filePath, 'utf-8')).trimStart(); const metadata = this.extractFrontmatter(rawContent, filePath); const body = this.stripFrontmatter(rawContent); - return { name: metadata.name, description: metadata.description, content: body }; + return { + name: typeof metadata.name === 'string' ? metadata.name : '', + description: typeof metadata.description === 'string' ? metadata.description : '', + content: body, + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/plugin/skill/skill-manager.ts` around lines 352 - 357, getSkillFileContent currently returns metadata fields as-is which can propagate non-string values; update getSkillFileContent to coerce/normalize metadata.name and metadata.description (from this.extractFrontmatter(rawContent, filePath)) into safe strings (e.g., ensure typeof === 'string' then trim, otherwise default to empty string) before assembling the SkillFileContent with the body from this.stripFrontmatter; this prevents non-string values from entering renderer form state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/main/src/plugin/skill/skill-manager.spec.ts`:
- Line 700: The test fixture uses a non-interpolated template literal
(EXTRA_SKILL_MD) which trips the lint rule; replace the backtick string with a
normal quoted string literal containing explicit \n escapes (e.g. const
EXTRA_SKILL_MD = '---\\nname: extra-skill\\ndescription: An extra
skill\\n---\\n# Extra\\n';) and make the same replacement for the other
non-interpolated template literal at the other occurrence (around the second
instance noted at line ~737) so both are plain string literals instead of
template literals.
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 287-300: discoverSkillsInDirectory runs async and can re-add
skills after dispose; fix by making discovery results conditional on the folder
still being registered and by early-cancelling stale runs. When starting
discoverSkillsInDirectory(folder) capture a unique key (e.g.,
folder.baseDirectory or a generated discoveryToken stored on the folder) and
check this token inside processDiscoveredPaths before merging; in dispose()
clear or increment that token / remove the folder from an activeDiscoveries map
so any in-flight discovery checks the active map and aborts without adding
skills. Also add a merge-time guard in processDiscoveredPaths that verifies the
resolved discovery root is still present in this.skillFolders (matching
baseDirectory) before accepting discovered entries.
In `@packages/renderer/src/lib/skills/SkillFolderCards.svelte`:
- Line 15: The current initialization of folders uses await
window.listSkillFolders() directly and can reject, breaking the component; wrap
the call in a try/catch (or use Promise.resolve(...).catch(() => [])) so that if
window.listSkillFolders() throws you fall back to an empty array and still
create the derived store (i.e., ensure const folders = $derived(... ) receives
[] on error), and optionally log the error; update references to folders and
window.listSkillFolders() accordingly so the dialog remains interactive when
listing fails.
---
Duplicate comments:
In `@packages/main/src/plugin/skill/skill-manager.ts`:
- Around line 352-357: getSkillFileContent currently returns metadata fields
as-is which can propagate non-string values; update getSkillFileContent to
coerce/normalize metadata.name and metadata.description (from
this.extractFrontmatter(rawContent, filePath)) into safe strings (e.g., ensure
typeof === 'string' then trim, otherwise default to empty string) before
assembling the SkillFileContent with the body from this.stripFrontmatter; this
prevents non-string values from entering renderer form state.
In `@packages/renderer/src/lib/skills/SkillCreate.svelte`:
- Around line 81-85: prefillFromParsed assigns unvalidated parsed values
directly to form state (name, description, skillContent); guard these by
checking the type/shape before assignment (e.g., ensure parsed.name and
parsed.description are strings and parsed.content is a string or convert/default
to ""), so update the prefillFromParsed function to only assign when typeof
parsed.name === "string" (and similarly for description and content) or
otherwise set a safe default to prevent runtime validation errors in downstream
code that expects strings.
- Around line 118-125: The catch block leaves selectedFile set while clearing
skillContent, causing inconsistent dialog state; update the error path in the
try/catch around window.getSkillFileContent(selected) so that when the parse
fails you also reset selectedFile (and any related state set by
prefillFromParsed) — e.g. set selectedFile to null/empty and clear any
parsed-derived fields alongside skillContent so the dialog reflects no selection
when parsing fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35e26190-1569-4edf-a9f0-fd65aef45df2
📒 Files selected for processing (10)
packages/api/src/skill/skill-info.tspackages/main/src/plugin/skill/skill-manager.spec.tspackages/main/src/plugin/skill/skill-manager.tspackages/preload/src/index.tspackages/renderer/src/lib/skills/SkillCreate.spec.tspackages/renderer/src/lib/skills/SkillCreate.sveltepackages/renderer/src/lib/skills/SkillFolderCards.sveltepackages/renderer/src/lib/skills/SkillsList.spec.tspackages/renderer/src/lib/ui/CardSelector.spec.tspackages/renderer/src/lib/ui/CardSelector.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/renderer/src/lib/ui/CardSelector.spec.ts
- packages/renderer/src/lib/ui/CardSelector.svelte
- packages/renderer/src/lib/skills/SkillCreate.spec.ts
|
|
||
| test('registerSkillFolder should discover skills from its directory after init', async () => { | ||
| const EXTRA_DIR = resolve('/extra/skills'); | ||
| const EXTRA_SKILL_MD = `---\nname: extra-skill\ndescription: An extra skill\n---\n# Extra\n`; |
There was a problem hiding this comment.
Replace non-interpolated template literals with string literals.
These fixtures trigger Biome lint/style/noUnusedTemplateLiteral.
🔧 Proposed fix
- const EXTRA_SKILL_MD = `---\nname: extra-skill\ndescription: An extra skill\n---\n# Extra\n`;
+ const EXTRA_SKILL_MD = '---\nname: extra-skill\ndescription: An extra skill\n---\n# Extra\n';- const EXTRA_SKILL_MD = `---\nname: extra-skill\ndescription: An extra skill\n---\n# Extra\n`;
+ const EXTRA_SKILL_MD = '---\nname: extra-skill\ndescription: An extra skill\n---\n# Extra\n';Also applies to: 737-737
🧰 Tools
🪛 Biome (2.4.7)
[error] 700-700: Do not use template literals if interpolation and special-character handling are not needed.
(lint/style/noUnusedTemplateLiteral)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/main/src/plugin/skill/skill-manager.spec.ts` at line 700, The test
fixture uses a non-interpolated template literal (EXTRA_SKILL_MD) which trips
the lint rule; replace the backtick string with a normal quoted string literal
containing explicit \n escapes (e.g. const EXTRA_SKILL_MD = '---\\nname:
extra-skill\\ndescription: An extra skill\\n---\\n# Extra\\n';) and make the
same replacement for the other non-interpolated template literal at the other
occurrence (around the second instance noted at line ~737) so both are plain
string literals instead of template literals.
|
|
||
| const FALLBACK_ICON = faBoxOpen; | ||
|
|
||
| const folders = $derived(await window.listSkillFolders()); |
There was a problem hiding this comment.
Handle folder-loading failures to keep the dialog usable.
If window.listSkillFolders() rejects, this initialization path can fail and prevent target selection UI from rendering. Add a safe fallback ([]) and keep the component interactive.
💡 Proposed hardening
-const folders = $derived(await window.listSkillFolders());
+const folders = $derived(await window.listSkillFolders().catch(() => []));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/skills/SkillFolderCards.svelte` at line 15, The
current initialization of folders uses await window.listSkillFolders() directly
and can reject, breaking the component; wrap the call in a try/catch (or use
Promise.resolve(...).catch(() => [])) so that if window.listSkillFolders()
throws you fall back to an empty array and still create the derived store (i.e.,
ensure const folders = $derived(... ) receives [] on error), and optionally log
the error; update references to folders and window.listSkillFolders()
accordingly so the dialog remains interactive when listing fails.
fbf61fb to
1e02644
Compare
benoitf
approved these changes
Mar 19, 2026
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
gastoner
force-pushed
the
create_skill_action
branch
from
1e02644 to
defaa93
Compare
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
3 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.
This PR adds an action to add a skill to either kortex or claude repository
You can specify everything manually or select a SKILL.md file to prefill the content (you can edit it later in the dialog)
Screencast_20260317_083030.webm
Closes #1037