feat: add pure-client snapshot skill foundation#68
Open
AmintaCCCP wants to merge 3 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughAdds snapshot types, storage, search and mutation utilities, a CLI skill/tool, App-level sync that persists snapshots to localStorage and (when available) via Electron IPC to a desktop file, and main/preload IPC handlers for snapshot file read/write. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as App (renderer)
participant Store as useAppStore
participant Service as SnapshotService
participant Main as Electron Main
participant FS as Filesystem / localStorage
Renderer->>Store: read repositories, customCategories, isAuthenticated
Store-->>Renderer: repositories, customCategories, isAuthenticated
alt isAuthenticated == true
Renderer->>Service: syncSnapshotToLocalStorage(repos, categories)
Service->>FS: write snapshot to localStorage
FS-->>Service: ok
Renderer->>Service: writeSnapshotToDesktopFile(snapshot)
alt window.electronAPI present
Service->>Main: invoke 'write-snapshot' via ipcRenderer
Main->>FS: write snapshot file to userData path
FS-->>Main: {ok, path}
Main-->>Service: {ok, path}
else fallback
Service->>FS: write snapshot to localStorage (fallback)
FS-->>Service: {ok}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
scripts/githubstars-cli.mjs (2)
103-106: Avoid mutating the input object.
saveSnapshotmutatessnapshot.exportedAtdirectly. While it works, mutating inputs can cause subtle bugs. Create a new object instead.♻️ Proposed fix
const saveSnapshot = (snapshotPath, snapshot) => { - snapshot.exportedAt = new Date().toISOString(); - fs.writeFileSync(snapshotPath, JSON.stringify(snapshot, null, 2)); + const updated = { ...snapshot, exportedAt: new Date().toISOString() }; + fs.writeFileSync(snapshotPath, JSON.stringify(updated, null, 2)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/githubstars-cli.mjs` around lines 103 - 106, The saveSnapshot function currently mutates the passed-in snapshot by setting snapshot.exportedAt; instead, create a new object (e.g., copy all properties from snapshot into a fresh object and add exportedAt = new Date().toISOString()) and pass that new object to fs.writeFileSync(JSON.stringify(...)), leaving the original snapshot unchanged; update function saveSnapshot to build and serialize this new object rather than assigning to snapshot.exportedAt.
7-19: Significant code duplication withsrc/core/searchRepositories.ts.The
normalize,repoText,scoreRepository,searchRepositories, anddedupefunctions are duplicated from the TypeScript modules. This creates maintenance burden - any algorithm changes must be synchronized in both places.Since this is a standalone ESM file that can't import TypeScript directly, consider either:
- Documenting the coupling explicitly
- Using a build step to generate the CLI from shared sources
- Accepting the duplication for now with a TODO comment
Also applies to: 21-33, 35-48, 50-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/githubstars-cli.mjs` around lines 7 - 19, This file duplicates logic from src/core/searchRepositories.ts (functions normalize, repoText, scoreRepository, searchRepositories, dedupe) causing maintenance drift; either replace it with a generated/shared module via your build step or, if you want a quick/safe short-term fix, add a clear TODO comment at the top of scripts/githubstars-cli.mjs that documents the coupling: list the duplicated functions (normalize, repoText, scoreRepository, searchRepositories, dedupe), reference src/core/searchRepositories.ts as the canonical source, and state the desired long-term resolution (use build-time generation or shared import) so future maintainers know to sync changes or remove duplication..github/workflows/build-cli.yml (2)
34-38: Artifact contains only the script without dependencies.The workflow uploads only
scripts/githubstars-cli.mjs, but consumers will need Node.js to run it. Consider documenting this in the artifact description or bundling with a README.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-cli.yml around lines 34 - 38, The uploaded artifact "githubstars-cli" currently only contains the script at path scripts/githubstars-cli.mjs and lacks documentation or runtime instructions; update the "Upload CLI artifact" step so the artifact includes a README (or license/docs) explaining Node.js/runtime prerequisites or bundle that README alongside scripts/githubstars-cli.mjs (i.e., add the README to the artifact name "githubstars-cli") and ensure the artifact description clearly states that Node.js is required to execute the script.
30-32: Verification step is ineffective with|| true.The
|| truesuffix suppresses all exit codes, making this step always pass regardless of CLI errors. If the intent is to validate the CLI runs correctly, this defeats the purpose.Consider either:
- Removing
|| trueto fail the build on CLI errors, or- Adding a meaningful validation like
node scripts/githubstars-cli.mjs --helpwith expected output verification.♻️ Proposed fix
- name: Verify CLI help path run: | - node scripts/githubstars-cli.mjs || true + node scripts/githubstars-cli.mjs search --help 2>&1 | head -5 + # Verify CLI at least parses without crashing on a simple command structure check🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-cli.yml around lines 30 - 32, The "Verify CLI help path" step currently uses `node scripts/githubstars-cli.mjs || true`, which masks failures; update this to actually validate the CLI by removing the `|| true` so the step fails on error, or replace it with an explicit verification such as running `node scripts/githubstars-cli.mjs --help` and asserting expected output (e.g., grep/assert on help text) so failures are detected; target the step name "Verify CLI help path" and the script invocation `node scripts/githubstars-cli.mjs` when making the change.skills/githubstars-cli/SKILL.md (1)
16-19: Clarify default snapshot path.The CLI defaults to
github-stars.snapshot.jsonin the current directory when--snapshotis omitted. Consider adding this to reduce user confusion:📝 Suggested addition
## Commands +Default snapshot path: `./github-stars.snapshot.json` (override with `--snapshot <path>`) + - `node scripts/githubstars-cli.mjs search "mcp server" --snapshot <path>`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/githubstars-cli/SKILL.md` around lines 16 - 19, Add a brief note to the SKILL.md commands section clarifying that the CLI uses "github-stars.snapshot.json" in the current working directory as the default snapshot file when the --snapshot option is omitted; mention this default alongside the existing command examples that reference scripts/githubstars-cli.mjs so users know they can omit --snapshot to use the local github-stars.snapshot.json file.src/core/repositoryMutations.ts (1)
3-17: Code duplication with CLI's dedupe implementation.This
dedupefunction is duplicated inscripts/githubstars-cli.mjs(Lines 50-62). While the CLI is a standalone .mjs file that can't easily import TypeScript modules, the duplication creates a maintenance burden if the deduplication logic needs to change.Consider documenting this coupling or extracting to a shared location if the build process evolves to support it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/repositoryMutations.ts` around lines 3 - 17, The dedupe function is duplicated (the dedupe implementation here and the CLI's dedupe in the .mjs script); either extract the logic into a single shared utility (export a dedupe function from a new/common util module and import it where possible, e.g., replace the local dedupe in repositoryMutations.ts with that exported function) or, if the CLI cannot yet import TypeScript, add a short comment in repositoryMutations.ts and in the CLI next to its dedupe implementation documenting the coupling and the intended future refactor so maintainers know to update both places when changing deduplication logic.
🤖 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/githubstars-cli.mjs`:
- Around line 96-101: The loadSnapshot function currently calls JSON.parse on
the file contents without guarding against malformed JSON; wrap the JSON.parse
call in a try/catch inside loadSnapshot (using snapshotPath to read the file)
and when parsing fails throw or return a consistent, structured error message
(e.g., include snapshotPath and the parsing error.message) so the CLI produces
the same error shape as other failures instead of surfacing a raw SyntaxError.
- Line 132: The current assignment "const limit = Number(flags.get('--limit') ||
20);" incorrectly treats "--limit 0" as 20; change it to use nullish coalescing
so only undefined/null falls back (e.g., Number(flags.get('--limit') ?? 20)),
and add explicit validation after parsing (check Number.isFinite(limit) and
optionally enforce limit > 0 or throw a user-facing error) so "--limit 0" is
handled as 0 or rejected per intended behavior; update the code referencing the
limit variable accordingly.
In `@src/App.tsx`:
- Around line 40-44: The app currently only mirrors the snapshot to localStorage
via useEffect + syncSnapshotToLocalStorage (dependent on isAuthenticated,
repositories, customCategories) but provides no way to save it to disk for CLI
consumption; add an "Export Snapshot" button to SettingsPanel that, when
clicked, grabs the same snapshot payload (use the same source used by
syncSnapshotToLocalStorage or expose a helper like getSnapshot(repositories,
customCategories)), stringifies it and triggers a browser download named
github-stars.snapshot.json (application/json, blob URL) so the CLI can read the
file; wire the button into SettingsPanel UI and ensure it is only enabled when
isAuthenticated and snapshot data exists.
In `@src/core/searchRepositories.ts`:
- Around line 45-48: The total field is inconsistent between branches in
searchRepositories: when query is empty you return total = repositories.length
(collection size) but when query is used you set total to matched.length
(post-filter size), so decide which semantics you want and make both branches
consistent; either (A) make total represent "matches before limit" by computing
matchedBeforeSlice = matched (or repositories when query empty) and returning
total = matchedBeforeSlice.length, or (B) make total represent "collection size"
by always returning total = repositories.length—update the early-return branch
and the query branch accordingly in the searchRepositories function (variables:
repositories, matched, sliced, limit, total) so both branches follow the chosen
definition.
---
Nitpick comments:
In @.github/workflows/build-cli.yml:
- Around line 34-38: The uploaded artifact "githubstars-cli" currently only
contains the script at path scripts/githubstars-cli.mjs and lacks documentation
or runtime instructions; update the "Upload CLI artifact" step so the artifact
includes a README (or license/docs) explaining Node.js/runtime prerequisites or
bundle that README alongside scripts/githubstars-cli.mjs (i.e., add the README
to the artifact name "githubstars-cli") and ensure the artifact description
clearly states that Node.js is required to execute the script.
- Around line 30-32: The "Verify CLI help path" step currently uses `node
scripts/githubstars-cli.mjs || true`, which masks failures; update this to
actually validate the CLI by removing the `|| true` so the step fails on error,
or replace it with an explicit verification such as running `node
scripts/githubstars-cli.mjs --help` and asserting expected output (e.g.,
grep/assert on help text) so failures are detected; target the step name "Verify
CLI help path" and the script invocation `node scripts/githubstars-cli.mjs` when
making the change.
In `@scripts/githubstars-cli.mjs`:
- Around line 103-106: The saveSnapshot function currently mutates the passed-in
snapshot by setting snapshot.exportedAt; instead, create a new object (e.g.,
copy all properties from snapshot into a fresh object and add exportedAt = new
Date().toISOString()) and pass that new object to
fs.writeFileSync(JSON.stringify(...)), leaving the original snapshot unchanged;
update function saveSnapshot to build and serialize this new object rather than
assigning to snapshot.exportedAt.
- Around line 7-19: This file duplicates logic from
src/core/searchRepositories.ts (functions normalize, repoText, scoreRepository,
searchRepositories, dedupe) causing maintenance drift; either replace it with a
generated/shared module via your build step or, if you want a quick/safe
short-term fix, add a clear TODO comment at the top of
scripts/githubstars-cli.mjs that documents the coupling: list the duplicated
functions (normalize, repoText, scoreRepository, searchRepositories, dedupe),
reference src/core/searchRepositories.ts as the canonical source, and state the
desired long-term resolution (use build-time generation or shared import) so
future maintainers know to sync changes or remove duplication.
In `@skills/githubstars-cli/SKILL.md`:
- Around line 16-19: Add a brief note to the SKILL.md commands section
clarifying that the CLI uses "github-stars.snapshot.json" in the current working
directory as the default snapshot file when the --snapshot option is omitted;
mention this default alongside the existing command examples that reference
scripts/githubstars-cli.mjs so users know they can omit --snapshot to use the
local github-stars.snapshot.json file.
In `@src/core/repositoryMutations.ts`:
- Around line 3-17: The dedupe function is duplicated (the dedupe implementation
here and the CLI's dedupe in the .mjs script); either extract the logic into a
single shared utility (export a dedupe function from a new/common util module
and import it where possible, e.g., replace the local dedupe in
repositoryMutations.ts with that exported function) or, if the CLI cannot yet
import TypeScript, add a short comment in repositoryMutations.ts and in the CLI
next to its dedupe implementation documenting the coupling and the intended
future refactor so maintainers know to update both places when changing
deduplication logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8431ddc-c799-41f7-bd7d-07643a29db82
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/build-cli.ymlscripts/githubstars-cli.mjsskills/githubstars-cli/SKILL.mdsrc/App.tsxsrc/core/repositoryMutations.tssrc/core/searchRepositories.tssrc/core/snapshotTypes.tssrc/services/snapshotStorage.ts
Comment on lines
+96
to
+101
| const loadSnapshot = (snapshotPath) => { | ||
| if (!fs.existsSync(snapshotPath)) { | ||
| throw new Error(`Snapshot not found: ${snapshotPath}`); | ||
| } | ||
| return JSON.parse(fs.readFileSync(snapshotPath, 'utf8')); | ||
| }; |
There was a problem hiding this comment.
Add error handling for malformed JSON.
If the snapshot file contains invalid JSON, JSON.parse throws a raw SyntaxError that bypasses your structured error response. Wrap in try/catch for consistent error output.
🛡️ Proposed fix
const loadSnapshot = (snapshotPath) => {
if (!fs.existsSync(snapshotPath)) {
throw new Error(`Snapshot not found: ${snapshotPath}`);
}
- return JSON.parse(fs.readFileSync(snapshotPath, 'utf8'));
+ try {
+ return JSON.parse(fs.readFileSync(snapshotPath, 'utf8'));
+ } catch (err) {
+ throw new Error(`Failed to parse snapshot: ${err.message}`);
+ }
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/githubstars-cli.mjs` around lines 96 - 101, The loadSnapshot function
currently calls JSON.parse on the file contents without guarding against
malformed JSON; wrap the JSON.parse call in a try/catch inside loadSnapshot
(using snapshotPath to read the file) and when parsing fails throw or return a
consistent, structured error message (e.g., include snapshotPath and the parsing
error.message) so the CLI produces the same error shape as other failures
instead of surfacing a raw SyntaxError.
| const { flags, positional } = parseArgs(args.slice(1)); | ||
| const snapshotPath = getSnapshotPath(flags); | ||
| const query = positional.join(' ').trim(); | ||
| const limit = Number(flags.get('--limit') || 20); |
There was a problem hiding this comment.
--limit 0 silently becomes 20.
Number(undefined) returns NaN, and Number("0") returns 0. With || 20, both become 20. If --limit 0 should be invalid, add explicit validation. If it should return no results, use nullish coalescing (??).
🛡️ Proposed fix using nullish coalescing
- const limit = Number(flags.get('--limit') || 20);
+ const limitRaw = flags.get('--limit');
+ const limit = typeof limitRaw === 'string' ? Number(limitRaw) : 20;
+ if (Number.isNaN(limit) || limit < 0) throw new Error('--limit must be a non-negative number');📝 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 limit = Number(flags.get('--limit') || 20); | |
| const limitRaw = flags.get('--limit'); | |
| const limit = typeof limitRaw === 'string' ? Number(limitRaw) : 20; | |
| if (Number.isNaN(limit) || limit < 0) throw new Error('--limit must be a non-negative number'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/githubstars-cli.mjs` at line 132, The current assignment "const limit
= Number(flags.get('--limit') || 20);" incorrectly treats "--limit 0" as 20;
change it to use nullish coalescing so only undefined/null falls back (e.g.,
Number(flags.get('--limit') ?? 20)), and add explicit validation after parsing
(check Number.isFinite(limit) and optionally enforce limit > 0 or throw a
user-facing error) so "--limit 0" is handled as 0 or rejected per intended
behavior; update the code referencing the limit variable accordingly.
Comment on lines
+45
to
+48
| if (!query) { | ||
| const sliced = repositories.slice(0, limit); | ||
| return { repositories: sliced, total: repositories.length, query }; | ||
| } |
There was a problem hiding this comment.
Inconsistent semantics of total field.
When query is empty, total is set to repositories.length (the full collection size). When query matches, total equals matched.length (the matched subset size, same as repositories.length in the result). This inconsistency could confuse API consumers.
Consider clarifying the intent:
- If
totalshould always represent "total matches before limit", both branches are correct. - If
totalshould represent "total in collection", the query branch should usematchedBeforeSlice.length.
🔍 If total should mean "matches before limit"
const matched = repositories
.map((repo) => ({ repo, score: scoreRepository(repo, queryWords) }))
.filter((item) => item.score > 0)
- .sort((a, b) => b.score - a.score || b.repo.stargazers_count - a.repo.stargazers_count)
- .slice(0, limit)
- .map((item) => item.repo);
+ .sort((a, b) => b.score - a.score || b.repo.stargazers_count - a.repo.stargazers_count);
+
+ const totalMatched = matched.length;
+ const sliced = matched.slice(0, limit).map((item) => item.repo);
return {
- repositories: matched,
- total: matched.length,
+ repositories: sliced,
+ total: totalMatched,
query,
};Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/searchRepositories.ts` around lines 45 - 48, The total field is
inconsistent between branches in searchRepositories: when query is empty you
return total = repositories.length (collection size) but when query is used you
set total to matched.length (post-filter size), so decide which semantics you
want and make both branches consistent; either (A) make total represent "matches
before limit" by computing matchedBeforeSlice = matched (or repositories when
query empty) and returning total = matchedBeforeSlice.length, or (B) make total
represent "collection size" by always returning total =
repositories.length—update the early-return branch and the query branch
accordingly in the searchRepositories function (variables: repositories,
matched, sliced, limit, total) so both branches follow the chosen definition.
AmintaCCCP
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/githubstars-snapshot/scripts/githubstars-snapshot-tool.mjs`:
- Around line 143-154: The category extraction can incorrectly accept a boolean
when a value is missing; change the logic in the "command === 'category' &&
subcommand === 'set'" block to mirror the repo handling: check typeof
flags.get('--category') === 'string' before using it (e.g., assign category from
String(flags.get('--category')) only when that type check passes), so that a
bare "--category" flag doesn't become "true"; keep the existing validation that
throws the Error('category set requires --repo and --category') if either is
missing and then proceed to use snapshot, findByRepo, replaceRepository,
saveSnapshot and printJson as before.
- Around line 156-171: The tags parsing treats a bare "--tags" flag (which
yields boolean true) as the string "true"; update the parsing in the command ===
'tags' && subcommand === 'add' block so you only split when flags.get('--tags')
is a string: get the rawTags = flags.get('--tags'); if rawTags === true or
rawTags == null treat it as missing (throw the existing "tags add requires
--repo and --tags" error) or set to empty, otherwise compute tags =
String(rawTags).split(',').map(...).filter(Boolean); adjust the earlier presence
check to use typeof rawTags === 'string' (or flags.has('--tags') && typeof
rawTags === 'string') so findByRepo, dedupe, replaceRepository, saveSnapshot,
and printJson continue to operate on a correct tags array.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d70041d-cb35-40c5-b8a5-4c4fcb862269
📒 Files selected for processing (2)
skills/githubstars-snapshot/SKILL.mdskills/githubstars-snapshot/scripts/githubstars-snapshot-tool.mjs
✅ Files skipped from review due to trivial changes (1)
- skills/githubstars-snapshot/SKILL.md
Comment on lines
+143
to
+154
| if (command === 'category' && subcommand === 'set') { | ||
| const snapshot = loadSnapshot(snapshotPath); | ||
| const category = String(flags.get('--category') || '').trim(); | ||
| const repoName = typeof flags.get('--repo') === 'string' ? String(flags.get('--repo')) : ''; | ||
| if (!category || !repoName) throw new Error('category set requires --repo and --category'); | ||
| const repo = findByRepo(snapshot.repositories, repoName); | ||
| const updated = { ...repo, custom_category: category, last_edited: new Date().toISOString() }; | ||
| snapshot.repositories = replaceRepository(snapshot.repositories, updated); | ||
| if (!flags.has('--dry-run')) saveSnapshot(snapshotPath, snapshot); | ||
| printJson({ ok: true, action: 'category.set', dryRun: flags.has('--dry-run'), repository: updated }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Inconsistent flag extraction: --category doesn't guard against boolean.
Line 146 correctly checks typeof flags.get('--repo') === 'string', but line 145 uses String(flags.get('--category') || ''). If a user passes --category --dry-run (missing value), --category becomes true (boolean), and String(true) yields "true" as the category.
🛡️ Proposed fix for consistency
- const category = String(flags.get('--category') || '').trim();
+ const category = typeof flags.get('--category') === 'string' ? flags.get('--category').trim() : '';📝 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
| if (command === 'category' && subcommand === 'set') { | |
| const snapshot = loadSnapshot(snapshotPath); | |
| const category = String(flags.get('--category') || '').trim(); | |
| const repoName = typeof flags.get('--repo') === 'string' ? String(flags.get('--repo')) : ''; | |
| if (!category || !repoName) throw new Error('category set requires --repo and --category'); | |
| const repo = findByRepo(snapshot.repositories, repoName); | |
| const updated = { ...repo, custom_category: category, last_edited: new Date().toISOString() }; | |
| snapshot.repositories = replaceRepository(snapshot.repositories, updated); | |
| if (!flags.has('--dry-run')) saveSnapshot(snapshotPath, snapshot); | |
| printJson({ ok: true, action: 'category.set', dryRun: flags.has('--dry-run'), repository: updated }); | |
| return; | |
| } | |
| if (command === 'category' && subcommand === 'set') { | |
| const snapshot = loadSnapshot(snapshotPath); | |
| const category = typeof flags.get('--category') === 'string' ? flags.get('--category').trim() : ''; | |
| const repoName = typeof flags.get('--repo') === 'string' ? String(flags.get('--repo')) : ''; | |
| if (!category || !repoName) throw new Error('category set requires --repo and --category'); | |
| const repo = findByRepo(snapshot.repositories, repoName); | |
| const updated = { ...repo, custom_category: category, last_edited: new Date().toISOString() }; | |
| snapshot.repositories = replaceRepository(snapshot.repositories, updated); | |
| if (!flags.has('--dry-run')) saveSnapshot(snapshotPath, snapshot); | |
| printJson({ ok: true, action: 'category.set', dryRun: flags.has('--dry-run'), repository: updated }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/githubstars-snapshot/scripts/githubstars-snapshot-tool.mjs` around
lines 143 - 154, The category extraction can incorrectly accept a boolean when a
value is missing; change the logic in the "command === 'category' && subcommand
=== 'set'" block to mirror the repo handling: check typeof
flags.get('--category') === 'string' before using it (e.g., assign category from
String(flags.get('--category')) only when that type check passes), so that a
bare "--category" flag doesn't become "true"; keep the existing validation that
throws the Error('category set requires --repo and --category') if either is
missing and then proceed to use snapshot, findByRepo, replaceRepository,
saveSnapshot and printJson as before.
Comment on lines
+156
to
+171
| if (command === 'tags' && subcommand === 'add') { | ||
| const snapshot = loadSnapshot(snapshotPath); | ||
| const tags = String(flags.get('--tags') || '').split(',').map((item) => item.trim()).filter(Boolean); | ||
| const repoName = typeof flags.get('--repo') === 'string' ? String(flags.get('--repo')) : ''; | ||
| if (!tags.length || !repoName) throw new Error('tags add requires --repo and --tags'); | ||
| const repo = findByRepo(snapshot.repositories, repoName); | ||
| const updated = { | ||
| ...repo, | ||
| custom_tags: dedupe([...(repo.custom_tags || []), ...tags]), | ||
| last_edited: new Date().toISOString(), | ||
| }; | ||
| snapshot.repositories = replaceRepository(snapshot.repositories, updated); | ||
| if (!flags.has('--dry-run')) saveSnapshot(snapshotPath, snapshot); | ||
| printJson({ ok: true, action: 'tags.add', dryRun: flags.has('--dry-run'), repository: updated }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same boolean-guard inconsistency for --tags.
If --tags is passed without a value, it becomes true (boolean), and String(true).split(',') yields ["true"] as a tag.
🛡️ Proposed fix for consistency
- const tags = String(flags.get('--tags') || '').split(',').map((item) => item.trim()).filter(Boolean);
+ const tagsRaw = flags.get('--tags');
+ const tags = typeof tagsRaw === 'string' ? tagsRaw.split(',').map((item) => item.trim()).filter(Boolean) : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/githubstars-snapshot/scripts/githubstars-snapshot-tool.mjs` around
lines 156 - 171, The tags parsing treats a bare "--tags" flag (which yields
boolean true) as the string "true"; update the parsing in the command === 'tags'
&& subcommand === 'add' block so you only split when flags.get('--tags') is a
string: get the rawTags = flags.get('--tags'); if rawTags === true or rawTags ==
null treat it as missing (throw the existing "tags add requires --repo and
--tags" error) or set to empty, otherwise compute tags =
String(rawTags).split(',').map(...).filter(Boolean); adjust the earlier presence
check to use typeof rawTags === 'string' (or flags.has('--tags') && typeof
rawTags === 'string') so findByRepo, dedupe, replaceRepository, saveSnapshot,
and printJson continue to operate on a correct tags array.
…le state - Add Electron IPC bridge (preload.js) so desktop app can write snapshot to fixed file - snapshotStorage.ts now writes to desktop app userData path via electronAPI - SKILL.md rewritten with real paths, commands, schema, and usage workflow - Skill now matches skill-creator规范: frontmatter + body + bundled tool
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
scripts/build-desktop.js (1)
77-104: Prefer async fs in these main-process handlers.Node documents that synchronous
fsAPIs block the event loop, whilefs/promisesmoves file operations off that thread. UsingmkdirSync/writeFileSync/readFileSynchere can freeze the desktop window during snapshot syncs as the snapshot grows. (nodejs.org)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-desktop.js` around lines 77 - 104, Replace blocking sync fs calls in the ipcMain handlers for 'write-snapshot' and 'read-snapshot' with async fs/promises APIs: in the 'write-snapshot' handler (which calls getSnapshotPath()) use fs.promises.mkdir(dir, { recursive: true }) and fs.promises.writeFile(snapshotPath, content, 'utf8'); in the 'read-snapshot' handler use fs.promises.access or fs.promises.stat to check existence and fs.promises.readFile(snapshotPath, 'utf8') to load content, then JSON.parse as before; keep the same return shape ({ok:true/false, path, data/error}) and preserve try/catch error handling around the awaited calls so the main process remains non-blocking.
🤖 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/build-desktop.js`:
- Line 40: The snapshot IPC handlers registered with ipcMain.handle (the
snapshot read/write handlers introduced around lines 77-109) must validate the
caller origin and use non-blocking file I/O: in each ipcMain.handle callback,
check event.senderFrame?.url (or derive origin via new
URL(event.senderFrame.url).origin) and compare it to an explicit trusted origin
(e.g., the app's known origin stored in a constant or computed from the main
window's webContents.getURL()) and immediately throw/return an error if it
doesn't match; then convert the handlers to async functions that use
fs.promises.writeFile and fs.promises.readFile (or otherwise run synchronous ops
off the main loop) and await results, and ensure errors are caught and rethrown
as IPC errors so the renderer receives a clear failure.
In `@src/services/snapshotStorage.ts`:
- Around line 58-63: getSnapshotPath currently returns SNAPSHOT_STORAGE_KEY (not
a filesystem path) when electronAPI is absent, which misleads callers and can
feed an invalid --snapshot argument; change getSnapshotPath (and its signature
if needed) to return an explicit non-file sentinel (e.g., null or an empty
string or a clear constant like "NO_SNAPSHOT_PATH") instead of
SNAPSHOT_STORAGE_KEY, and ensure callers that forward the result to the bundled
snapshot tool check for that sentinel before passing it through; update
references to getSnapshotPath, SNAPSHOT_STORAGE_KEY, and
electronAPI.getSnapshotPath accordingly so no code treats the storage key as a
filesystem path.
- Around line 24-33: The parsed payload is cast to GithubStarsSnapshot without
validation, causing stale/invalid data to propagate; update
readSnapshotFromLocalStorage (and the similar read path around lines 47-55) to
perform a small runtime guard after JSON.parse: verify the result is an object,
has the expected 'version' field, and that 'repositories' and 'categories' are
arrays (e.g., Array.isArray(snapshot.repositories) &&
Array.isArray(snapshot.categories)); if the checks fail return null (or treat as
not-found) instead of casting and returning; keep the existing try/catch around
JSON.parse and only return the parsed value after the guard passes.
- Around line 15-18: writeSnapshotToLocalStorage currently calls
window.localStorage.setItem without handling Storage errors; wrap the setItem
call in a try-catch and change writeSnapshotToLocalStorage to return a result
object (e.g., { ok: true } on success or { ok: false, error }) so callers
preserve the expected `{ ok, ... }` contract; catch QuotaExceededError (and any
other thrown error), populate the error in the returned object, and update
callers (writeSnapshotToDesktopFile, syncSnapshotToLocalStorage, and the direct
App.tsx calls) to handle the returned { ok: false, error } instead of allowing
an unhandled exception.
---
Nitpick comments:
In `@scripts/build-desktop.js`:
- Around line 77-104: Replace blocking sync fs calls in the ipcMain handlers for
'write-snapshot' and 'read-snapshot' with async fs/promises APIs: in the
'write-snapshot' handler (which calls getSnapshotPath()) use
fs.promises.mkdir(dir, { recursive: true }) and
fs.promises.writeFile(snapshotPath, content, 'utf8'); in the 'read-snapshot'
handler use fs.promises.access or fs.promises.stat to check existence and
fs.promises.readFile(snapshotPath, 'utf8') to load content, then JSON.parse as
before; keep the same return shape ({ok:true/false, path, data/error}) and
preserve try/catch error handling around the awaited calls so the main process
remains non-blocking.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 888a2cb2-213f-43a9-835f-ee55ceed73df
📒 Files selected for processing (4)
scripts/build-desktop.jsskills/githubstars-snapshot/SKILL.mdsrc/App.tsxsrc/services/snapshotStorage.ts
✅ Files skipped from review due to trivial changes (1)
- skills/githubstars-snapshot/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/App.tsx
| enableRemoteModule: false, | ||
| webSecurity: true | ||
| webSecurity: true, | ||
| preload: path.join(__dirname, 'preload.js'), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/build-desktop.js | head -160Repository: AmintaCCCP/GithubStarsManager
Length of output: 5317
Gate these IPC channels by trusted renderer origin and use async file operations.
The new ipcMain.handle() calls for snapshot read/write operations (lines 77-109) do not validate the sender via event.senderFrame, leaving them accessible to any code executing in the renderer context. Although contextIsolation and the preload bridge enforce basic isolation, an XSS vulnerability or injected content could reach these handlers. Per Electron security recommendations, validate that IPC calls originate from the expected frame origin. Additionally, lines 85 and 99 use fs.writeFileSync() and fs.readFileSync(), which block the main process; use async variants instead (or wrap in setImmediate).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build-desktop.js` at line 40, The snapshot IPC handlers registered
with ipcMain.handle (the snapshot read/write handlers introduced around lines
77-109) must validate the caller origin and use non-blocking file I/O: in each
ipcMain.handle callback, check event.senderFrame?.url (or derive origin via new
URL(event.senderFrame.url).origin) and compare it to an explicit trusted origin
(e.g., the app's known origin stored in a constant or computed from the main
window's webContents.getURL()) and immediately throw/return an error if it
doesn't match; then convert the handlers to async functions that use
fs.promises.writeFile and fs.promises.readFile (or otherwise run synchronous ops
off the main loop) and await results, and ensure errors are caught and rethrown
as IPC errors so the renderer receives a clear failure.
Comment on lines
+15
to
+18
| export function writeSnapshotToLocalStorage(snapshot: GithubStarsSnapshot): void { | ||
| if (typeof window === 'undefined') return; | ||
| window.localStorage.setItem(SNAPSHOT_STORAGE_KEY, JSON.stringify(snapshot)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/services/snapshotStorage.tsRepository: AmintaCCCP/GithubStarsManager
Length of output: 3193
🏁 Script executed:
rg "writeSnapshotToLocalStorage|syncSnapshotToLocalStorage" --type ts --type tsx -B 2 -A 2Repository: AmintaCCCP/GithubStarsManager
Length of output: 100
🏁 Script executed:
rg "writeSnapshotToLocalStorage|syncSnapshotToLocalStorage" --type ts -B 2 -A 2Repository: AmintaCCCP/GithubStarsManager
Length of output: 1727
🏁 Script executed:
sed -n '1,50p' src/App.tsx | cat -nRepository: AmintaCCCP/GithubStarsManager
Length of output: 2231
Wrap setItem() in try-catch to preserve the error contract.
Storage.setItem() throws QuotaExceededError when Web Storage quota is exceeded. In writeSnapshotToDesktopFile(), the browser fallback (lines 40–41) calls writeSnapshotToLocalStorage() without error handling, converting a quota exceeded error into an unhandled exception that rejects the Promise instead of returning { ok: false, error: ... }. Similarly, syncSnapshotToLocalStorage() (line 21) and direct calls in App.tsx (line 43) lack error handling and will crash if quota is exceeded.
Update writeSnapshotToLocalStorage() to catch and handle the exception, or add try-catch at call sites that expect a { ok, ... } contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/snapshotStorage.ts` around lines 15 - 18,
writeSnapshotToLocalStorage currently calls window.localStorage.setItem without
handling Storage errors; wrap the setItem call in a try-catch and change
writeSnapshotToLocalStorage to return a result object (e.g., { ok: true } on
success or { ok: false, error }) so callers preserve the expected `{ ok, ... }`
contract; catch QuotaExceededError (and any other thrown error), populate the
error in the returned object, and update callers (writeSnapshotToDesktopFile,
syncSnapshotToLocalStorage, and the direct App.tsx calls) to handle the returned
{ ok: false, error } instead of allowing an unhandled exception.
Comment on lines
+24
to
+33
| export function readSnapshotFromLocalStorage(): GithubStarsSnapshot | null { | ||
| if (typeof window === 'undefined') return null; | ||
| const raw = window.localStorage.getItem(SNAPSHOT_STORAGE_KEY); | ||
| if (!raw) return null; | ||
| try { | ||
| return JSON.parse(raw) as GithubStarsSnapshot; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate persisted data before returning it as GithubStarsSnapshot.
These read paths accept any parsed value and cast it straight to the snapshot type. A stale or hand-edited payload with the wrong version or missing repositories / categories will look type-safe here and fail later in unrelated code. Add a small runtime guard and reject mismatches before returning.
Also applies to: 47-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/snapshotStorage.ts` around lines 24 - 33, The parsed payload is
cast to GithubStarsSnapshot without validation, causing stale/invalid data to
propagate; update readSnapshotFromLocalStorage (and the similar read path around
lines 47-55) to perform a small runtime guard after JSON.parse: verify the
result is an object, has the expected 'version' field, and that 'repositories'
and 'categories' are arrays (e.g., Array.isArray(snapshot.repositories) &&
Array.isArray(snapshot.categories)); if the checks fail return null (or treat as
not-found) instead of casting and returning; keep the existing try/catch around
JSON.parse and only return the parsed value after the guard passes.
Comment on lines
+58
to
+63
| export async function getSnapshotPath(): Promise<string> { | ||
| const w = window as Window & { electronAPI?: { getSnapshotPath: () => Promise<string> } }; | ||
| if (!w.electronAPI) { | ||
| // Not in Electron, return localStorage key as fallback indicator | ||
| return SNAPSHOT_STORAGE_KEY; | ||
| } |
There was a problem hiding this comment.
Return an explicit non-file result instead of a storage key.
SNAPSHOT_STORAGE_KEY is not a filesystem path. Returning it from getSnapshotPath() makes the API contract misleading, and anything that forwards the value into the bundled snapshot tool will pass an unusable --snapshot argument.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/snapshotStorage.ts` around lines 58 - 63, getSnapshotPath
currently returns SNAPSHOT_STORAGE_KEY (not a filesystem path) when electronAPI
is absent, which misleads callers and can feed an invalid --snapshot argument;
change getSnapshotPath (and its signature if needed) to return an explicit
non-file sentinel (e.g., null or an empty string or a clear constant like
"NO_SNAPSHOT_PATH") instead of SNAPSHOT_STORAGE_KEY, and ensure callers that
forward the result to the bundled snapshot tool check for that sentinel before
passing it through; update references to getSnapshotPath, SNAPSHOT_STORAGE_KEY,
and electronAPI.getSnapshotPath accordingly so no code treats the storage key as
a filesystem path.
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
1 participant
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
Validation
npm run buildnode ./skills/githubstars-snapshot/scripts/githubstars-snapshot-tool.mjs search mcp --snapshot <snapshot-path>node ./skills/githubstars-snapshot/scripts/githubstars-snapshot-tool.mjs search "mcp server" --snapshot <snapshot-path>node ./skills/githubstars-snapshot/scripts/githubstars-snapshot-tool.mjs tags add --repo demo/awesome-mcp-server --tags mcp,agents --snapshot <snapshot-path> --dry-runnode ./skills/githubstars-snapshot/scripts/githubstars-snapshot-tool.mjs category set --repo demo/awesome-mcp-server --category ai-tools --snapshot <snapshot-path> --dry-runNotes
SKILL.mdso the skill is easier to use and distributeSummary by CodeRabbit
New Features
Documentation