chore: add svelte best practices skills#1158
Merged
bmahabirbu merged 2 commits intoopenkaiden:mainfrom Apr 3, 2026
Merged
Conversation
bmahabirbu
requested review from
fbricon and
gastoner
and removed request for
a team
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Svelte best-practices skills to the repo by installing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/svelte-core-bestpractices/references/@attach.md:
- Around line 149-155: The code example for function foo contains accidental
diff markers around the parameter and the $effect block; remove the `+++` tokens
so the signature reads `function foo(getBar)` and the inner block uses
`$effect(() => { update(node, getBar()); });` while preserving the existing
calls to veryExpensiveSetupWork(node), $effect, update, and getBar to restore a
valid, copy/pasteable example.
- Around line 28-147: Search the file for stray "(demo:" placeholders (e.g. in
the tooltip example, inline attachments block, wrapper component/Button example
and other occurrences) and replace each with a proper link to the demo page or
remove the "demo:" label entirely so the sentence reads correctly; ensure the
surrounding examples (functions like tooltip(content), the inline canvas
attachment, and the Button wrapper example) remain unchanged except for fixing
the broken "demo:" text.
In @.agents/skills/svelte-core-bestpractices/references/$inspect.md:
- Line 3: The sentence explaining $inspect is truncated and ends with "(demo:";
update the text in
.agents/skills/svelte-core-bestpractices/references/$inspect.md to complete the
sentence by either adding the missing demo link or removing the unfinished
parenthetical. Specifically, edit the paragraph mentioning the $inspect rune so
it reads as a complete statement (e.g., include a working demo URL or a
reference to an example component) and ensure the description of $inspect
tracking reactive state deeply remains clear and grammatically correct.
- Line 21: The sentence describing `$inspect` is truncated at "(demo:"—either
complete the sentence by adding the missing demo link or example code reference,
or remove the dangling "(demo:" phrase; update the markdown for `$inspect` so it
ends with a complete clause and, if adding a demo, include a valid link or
inline example that demonstrates `with` callbacks and the `"init"`/`"update"`
arguments.
In @.agents/skills/svelte-core-bestpractices/references/await-expressions.md:
- Line 117: The SSR example contains a stray diff marker around await; update
the expression in the example so the destructuring uses a plain await with the
render function (i.e., change the tokenized "+++await+++" to just await) so the
line reads the destructuring of { head, body } from await render(App)
referencing the render function and App component.
- Line 180: The sentence about effects running differently when the
experimental.async option is true is truncated; finish the thought to state the
concrete consequence (or remove the fragment). For example, update the sentence
referencing experimental.async, block effects like {`#if`} and {`#each`}, and hooks
$effect.pre / beforeUpdate to read something like: "Specifically, block effects
like {`#if` ...} and {`#each` ...} now run before an $effect.pre or beforeUpdate in
the same component, which means that in very rare situations a block effect may
observe or update DOM/state before those hooks run, potentially changing
timing-sensitive behavior." Alternatively, remove the trailing clause entirely
so the paragraph ends cleanly.
In @.agents/skills/svelte-core-bestpractices/references/snippet.md:
- Around line 56-255: Several occurrences of the placeholder text "(demo:" are
left incomplete in the snippet examples (e.g., near the {`#snippet` hello(name)}
example, the nested snippet visibility example, the countdown/blastoff examples,
the "Passing snippets to components" section with Table, the "Optional snippet
props" section, and the "Exporting snippets" section) — locate those occurrences
and either replace each "(demo:" with the correct target URL or example
reference (or a working inline demo link) or remove the stray "(demo:" text
entirely so the sentences read correctly; ensure you inspect examples
referencing snippet names like hello, x/y, blastoff/countdown, Table/header/row,
children, and the Snippet typing examples so no placeholder remains.
In `@package.json`:
- Line 67: The postinstall script in package.json currently uses the Unix-only
"cp -r"; replace it with a cross-platform copy tool (e.g., add cpx as a
devDependency) and update the "postinstall" npm script to use that tool to copy
node_modules/@sveltejs/opencode/skills/**/* into .agents/skills/ (keep the
existing electron-builder install-app-deps part). Modify the "postinstall"
script string in package.json (the "postinstall" key) and add the chosen package
to devDependencies so the copy works on Windows, macOS, and Linux.
🪄 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: f46f0c3b-a4b7-43d3-ad07-3c28d1b08703
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.agents/skills/svelte-code-writer/SKILL.md.agents/skills/svelte-core-bestpractices/SKILL.md.agents/skills/svelte-core-bestpractices/references/$inspect.md.agents/skills/svelte-core-bestpractices/references/@attach.md.agents/skills/svelte-core-bestpractices/references/@render.md.agents/skills/svelte-core-bestpractices/references/await-expressions.md.agents/skills/svelte-core-bestpractices/references/bind.md.agents/skills/svelte-core-bestpractices/references/each.md.agents/skills/svelte-core-bestpractices/references/hydratable.md.agents/skills/svelte-core-bestpractices/references/snippet.md.agents/skills/svelte-core-bestpractices/references/svelte-reactivity.mdpackage.json
| @@ -0,0 +1,53 @@ | |||
| > [!NOTE] `$inspect` only works during development. In a production build it becomes a noop. | |||
| The `$inspect` rune is roughly equivalent to `console.log`, with the exception that it will re-run whenever its argument changes. `$inspect` tracks reactive state deeply, meaning that updating something inside an object or array using fine-grained reactivity will cause it to re-fire (demo: | |||
Contributor
There was a problem hiding this comment.
Incomplete sentence - truncated demo link.
The sentence ends abruptly with "(demo:" which appears to be a truncated reference to a demo link. This incomplete text may confuse readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/svelte-core-bestpractices/references/$inspect.md at line 3,
The sentence explaining $inspect is truncated and ends with "(demo:"; update the
text in .agents/skills/svelte-core-bestpractices/references/$inspect.md to
complete the sentence by either adding the missing demo link or removing the
unfinished parenthetical. Specifically, edit the paragraph mentioning the
$inspect rune so it reads as a complete statement (e.g., include a working demo
URL or a reference to an example component) and ensure the description of
$inspect tracking reactive state deeply remains clear and grammatically correct.
| import { render } from 'svelte/server'; | ||
| import App from './App.svelte'; | ||
|
|
||
| const { head, body } = +++await+++ render(App); |
Contributor
There was a problem hiding this comment.
Fix the SSR example tokenization.
Line 117 contains +++await+++, which looks like a stray diff marker. The sample should use plain await.
Suggested fix
-const { head, body } = +++await+++ render(App);
+const { head, body } = await render(App);📝 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 { head, body } = +++await+++ render(App); | |
| const { head, body } = await render(App); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/svelte-core-bestpractices/references/await-expressions.md at
line 117, The SSR example contains a stray diff marker around await; update the
expression in the example so the destructuring uses a plain await with the
render function (i.e., change the tokenized "+++await+++" to just await) so the
line reads the destructuring of { head, body } from await render(App)
referencing the render function and App component.
|
|
||
| ## Breaking changes | ||
|
|
||
| Effects run in a slightly different order when the `experimental.async` option is `true`. Specifically, _block_ effects like `{#if ...}` and `{#each ...}` now run before an `$effect.pre` or `beforeUpdate` in the same component, which means that in very rare situations. |
Contributor
There was a problem hiding this comment.
Complete the truncated sentence.
Line 180 ends mid-thought (...which means that in very rare situations.), which makes the section unclear. Please complete or remove the trailing clause.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/svelte-core-bestpractices/references/await-expressions.md at
line 180, The sentence about effects running differently when the
experimental.async option is true is truncated; finish the thought to state the
concrete consequence (or remove the fragment). For example, update the sentence
referencing experimental.async, block effects like {`#if`} and {`#each`}, and hooks
$effect.pre / beforeUpdate to read something like: "Specifically, block effects
like {`#if` ...} and {`#each` ...} now run before an $effect.pre or beforeUpdate in
the same component, which means that in very rare situations a block effect may
observe or update DOM/state before those hooks run, potentially changing
timing-sensitive behavior." Alternatively, remove the trailing clause entirely
so the paragraph ends cleanly.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Add @sveltejs/opencode as a dev dependency and copy its skills (svelte-code-writer, svelte-core-bestpractices) into .agents/skills/ as a postinstall step. Fixes openkaiden#1118 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Brian <[email protected]>
bmahabirbu
force-pushed
the
svelte-skills
branch
from
c409b2e to
37ed7a4
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.agents/skills/svelte-core-bestpractices/references/await-expressions.md (2)
180-180:⚠️ Potential issue | 🟡 MinorComplete the truncated sentence.
Line 180 ends mid-thought (
...which means that in very rare situations.), leaving the consequence unclear. Please complete or remove the trailing clause.Note: This file is copied from
@sveltejs/opencode. Consider either applying a patch during postinstall, fixing upstream, or reporting the issue to the package maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/svelte-core-bestpractices/references/await-expressions.md at line 180, The sentence ending with "which means that in very rare situations." is incomplete; update the sentence in await-expressions.md to finish the thought or remove the trailing clause — e.g., clarify the consequence by completing it to: "which means that in very rare situations, DOM updates from block effects may occur before `$effect.pre` or `beforeUpdate` handlers run, potentially changing the timing assumptions in your component." Ensure you reference the `experimental.async` option and the behavior of block effects (`{`#if` ...}`, `{`#each` ...}`), `$effect.pre`, and `beforeUpdate` when making the change.
117-117:⚠️ Potential issue | 🟡 MinorFix the SSR example tokenization.
Line 117 contains
+++await+++, which appears to be stray diff markers. The example should use plainawait.🔧 Proposed fix
-const { head, body } = +++await+++ render(App); +const { head, body } = await render(App);Note: This file is copied from
@sveltejs/opencode. Consider either applying a patch during postinstall, fixing upstream, or reporting the issue to the package maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/svelte-core-bestpractices/references/await-expressions.md at line 117, Replace the stray diff markers around the await token in the SSR example so the expression reads a normal await call; specifically, in the example that destructures const { head, body } = +++await+++ render(App); remove the "+++...+++" tokens so it becomes a plain await before calling render(App) (look for the tokens "+++await+++" and the render(App) expression to locate the exact spot).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.agents/skills/svelte-core-bestpractices/references/await-expressions.md:
- Line 180: The sentence ending with "which means that in very rare situations."
is incomplete; update the sentence in await-expressions.md to finish the thought
or remove the trailing clause — e.g., clarify the consequence by completing it
to: "which means that in very rare situations, DOM updates from block effects
may occur before `$effect.pre` or `beforeUpdate` handlers run, potentially
changing the timing assumptions in your component." Ensure you reference the
`experimental.async` option and the behavior of block effects (`{`#if` ...}`,
`{`#each` ...}`), `$effect.pre`, and `beforeUpdate` when making the change.
- Line 117: Replace the stray diff markers around the await token in the SSR
example so the expression reads a normal await call; specifically, in the
example that destructures const { head, body } = +++await+++ render(App); remove
the "+++...+++" tokens so it becomes a plain await before calling render(App)
(look for the tokens "+++await+++" and the render(App) expression to locate the
exact spot).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d7264c6-0c3d-4398-b7fd-02faa03347ae
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.agents/skills/svelte-code-writer/SKILL.md.agents/skills/svelte-core-bestpractices/SKILL.md.agents/skills/svelte-core-bestpractices/references/$inspect.md.agents/skills/svelte-core-bestpractices/references/@attach.md.agents/skills/svelte-core-bestpractices/references/@render.md.agents/skills/svelte-core-bestpractices/references/await-expressions.md.agents/skills/svelte-core-bestpractices/references/bind.md.agents/skills/svelte-core-bestpractices/references/each.md.agents/skills/svelte-core-bestpractices/references/hydratable.md.agents/skills/svelte-core-bestpractices/references/snippet.md.agents/skills/svelte-core-bestpractices/references/svelte-reactivity.mdpackage.json
✅ Files skipped from review due to trivial changes (9)
- .agents/skills/svelte-core-bestpractices/references/bind.md
- .agents/skills/svelte-core-bestpractices/references/@render.md
- .agents/skills/svelte-core-bestpractices/references/each.md
- .agents/skills/svelte-code-writer/SKILL.md
- .agents/skills/svelte-core-bestpractices/references/svelte-reactivity.md
- .agents/skills/svelte-core-bestpractices/references/$inspect.md
- .agents/skills/svelte-core-bestpractices/references/hydratable.md
- .agents/skills/svelte-core-bestpractices/references/@attach.md
- .agents/skills/svelte-core-bestpractices/references/snippet.md
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
benoitf
requested changes
Mar 24, 2026
Contributor
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
AFAIK the files should not be committed
as it's done as post-install step, it's done locally
benoitf
reviewed
Mar 24, 2026
| "typecheck": "pnpm run typecheck:main && pnpm run typecheck:preload && pnpm run typecheck:renderer && pnpm run typecheck:preload-webview && pnpm run typecheck:extensions && pnpm run typecheck:extension-api", | ||
| "prepare": "husky install", | ||
| "postinstall": "electron-builder install-app-deps" | ||
| "postinstall": "electron-builder install-app-deps && node -e \"const fs=require('fs');fs.cpSync('node_modules/@sveltejs/opencode/skills','.agents/skills',{recursive:true})\"" |
Contributor
There was a problem hiding this comment.
there is cpy npmjs command or add a script to the scripts folder. Usually we don't inline stuff inside a script
(also require should no longer import 'fs' but 'node:fs')
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/copy-svelte-skills.js (1)
13-22: Scope copying to the two required skills to avoid future repo driftLines 13-22 currently copy the entire upstream
skillstree. If@sveltejs/opencodeadds more skill folders, postinstall may generate new local artifacts outside your explicit ignore list.💡 Proposed refactor (copy only required skills + validate presence)
-const source = path.resolve(__dirname, '..', 'node_modules', '@sveltejs', 'opencode', 'skills'); -const destination = path.resolve(__dirname, '..', '.agents', 'skills'); +const sourceRoot = path.resolve(__dirname, '..', 'node_modules', '@sveltejs', 'opencode', 'skills'); +const destinationRoot = path.resolve(__dirname, '..', '.agents', 'skills'); +const requiredSkills = ['svelte-code-writer', 'svelte-core-bestpractices']; -if (!existsSync(source)) { +if (!existsSync(sourceRoot)) { console.log('Skipping svelte skills copy: `@sveltejs/opencode` not installed.'); process.exit(0); } -cpSync(source, destination, { recursive: true }); -console.log('Copied svelte skills to .agents/skills/'); +for (const skill of requiredSkills) { + const source = path.join(sourceRoot, skill); + if (!existsSync(source)) { + throw new Error(`Missing expected skill in `@sveltejs/opencode`: ${skill}`); + } + cpSync(source, path.join(destinationRoot, skill), { recursive: true }); +} +console.log(`Copied ${requiredSkills.join(', ')} to .agents/skills/`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/copy-svelte-skills.js` around lines 13 - 22, The current script copies the entire skills tree (variables source/destination and cpSync call); change it to only copy the two required skill folders by defining an array of required skill names, for each skill construct its source path (using path.resolve with source base) and validate existence via existsSync, log and exit with non-zero if any required folder is missing, and copy each required folder individually to the destination (using cpSync or equivalent) instead of copying the whole source folder; keep existing console messages but make them specific per-skill and ensure destination directories are created as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/copy-svelte-skills.js`:
- Around line 10-11: The script currently uses CommonJS require(...) for cpSync
and existsSync and path which breaks when package.json declares "type":
"module"; replace the require calls with ESM import syntax (e.g., import {
cpSync, existsSync } from 'node:fs' and import path from 'node:path' or import {
dirname } from 'node:path' as needed) so the script runs as an ES module; update
any other CommonJS usages in the file (exports/module.exports or require) to ESM
equivalents if present.
---
Nitpick comments:
In `@scripts/copy-svelte-skills.js`:
- Around line 13-22: The current script copies the entire skills tree (variables
source/destination and cpSync call); change it to only copy the two required
skill folders by defining an array of required skill names, for each skill
construct its source path (using path.resolve with source base) and validate
existence via existsSync, log and exit with non-zero if any required folder is
missing, and copy each required folder individually to the destination (using
cpSync or equivalent) instead of copying the whole source folder; keep existing
console messages but make them specific per-skill and ensure destination
directories are created as needed.
🪄 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: 7ef75154-4a3d-41fe-be8b-554c3d75464f
📒 Files selected for processing (3)
.gitignorepackage.jsonscripts/copy-svelte-skills.js
✅ Files skipped from review due to trivial changes (1)
- .gitignore
benoitf
reviewed
Mar 27, 2026
Comment on lines
+10
to
+11
| const { cpSync, existsSync } = require('node:fs'); | ||
| const path = require('node:path'); |
Contributor
There was a problem hiding this comment.
should use import + copyright
benoitf
reviewed
Mar 27, 2026
| const source = path.resolve(__dirname, '..', 'node_modules', '@sveltejs', 'opencode', 'skills'); | ||
| const destination = path.resolve(__dirname, '..', '.agents', 'skills'); | ||
|
|
||
| if (!existsSync(source)) { |
Contributor
There was a problem hiding this comment.
can it use await / promise ?
gastoner
reviewed
Mar 30, 2026
| */ | ||
|
|
||
| const { cpSync, existsSync } = require('node:fs'); | ||
| const path = require('node:path'); |
Contributor
There was a problem hiding this comment.
Also I think we can import the relevant functions from the path itself
… generated files Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Brian <[email protected]>
bmahabirbu
force-pushed
the
svelte-skills
branch
from
93cb08b to
92458d1
Compare
benoitf
approved these changes
Apr 2, 2026
Contributor
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
checked and now it works
jeffmaury
approved these changes
Apr 2, 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
4 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.
Summary
@sveltejs/opencodeas a dev dependency to provide Svelte best practices skillspostinstallscript to copy skills (svelte-code-writer,svelte-core-bestpractices) fromnode_modules/@sveltejs/opencode/skills/into.agents/skills/Fixes #1118
Test plan
pnpm installand verify skills are copied to.agents/skills/svelte-code-writer/SKILL.mdandsvelte-core-bestpractices/SKILL.mdexist after installplaywright-testingskill is unaffected🤖 Generated with Claude Code