feat: show and sync complete Skills inventories#349
Conversation
📝 WalkthroughWalkthroughAdds per-device Skills inventory discovery, authenticated cloud storage and retrieval, cross-device merging, read-only UI handling, localized strings, and guardrail tests. ChangesSkills inventory synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SkillsPage
participant SkillsAPI
participant EdgeFunction
participant InventoryDatabase
SkillsPage->>SkillsAPI: request cloud inventory
SkillsAPI->>EdgeFunction: authenticated GET request
EdgeFunction->>InventoryDatabase: query active devices and inventories
InventoryDatabase-->>EdgeFunction: device inventory rows
EdgeFunction-->>SkillsAPI: devices payload
SkillsAPI-->>SkillsPage: cloud inventory
SkillsPage->>SkillsPage: merge local and cloud skills
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)dashboard/src/content/i18n/de/core.jsonTraceback (most recent call last): dashboard/src/content/i18n/ja/core.jsonTraceback (most recent call last): dashboard/src/content/i18n/ko/core.jsonTraceback (most recent call last):
🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dashboard/src/pages/SkillsPage.jsx (1)
1132-1140: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUse stable identity
skill.id || skill.directoryfor selection and busy states.Unmanaged local skills lack an
idand rely onskill.directoryfor identity. Usingskill.idexclusively for Set membership causes all unmanaged skills to map toundefined. Consequently, selecting one unmanaged skill visually selects all of them, and executing a bulk action (like remove) will unintentionally delete all unmanaged skills. Additionally, busy spinners will cross-contaminate across unmanaged rows.
dashboard/src/pages/SkillsPage.jsx#L1132-L1140: Useconst id = skill.id || skill.directory;and add/deleteidfrom thenextset.dashboard/src/pages/SkillsPage.jsx#L188-L188: Change tobusy={busyKey === targetBusyKey(skill.id || skill.directory, target.id)}.dashboard/src/pages/SkillsPage.jsx#L557-L558: Change tochecked={selectedIds.has(skill.id || skill.directory)}.dashboard/src/pages/SkillsPage.jsx#L1107-L1109: Change torunMutation(targetBusyKey(skill.id || skill.directory, targetId)...).dashboard/src/pages/SkillsPage.jsx#L1146-L1148: Change filter toselectedIds.has(s.id || s.directory).dashboard/src/pages/SkillsPage.jsx#L1168-L1170: Change filter toselectedIds.has(s.id || s.directory).dashboard/src/pages/SkillDetailPanel.jsx#L329-L329: Change tobusy={busyKey === targetBusyKey(skill.id || skill.directory, target.id)}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/src/pages/SkillsPage.jsx` around lines 1132 - 1140, Use the stable identity skill.id || skill.directory throughout selection and busy-state handling. In dashboard/src/pages/SkillsPage.jsx at lines 1132-1140, 188, 557-558, 1107-1109, 1146-1148, and 1168-1170, update handleToggleSelect, checked state, targetBusyKey calls, and selectedIds filters to use that fallback identity; make the corresponding busy-state update in dashboard/src/pages/SkillDetailPanel.jsx at line 329.
🧹 Nitpick comments (1)
dashboard/src/lib/skills-api.ts (1)
131-136: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a request timeout for the cloud fetch.
fetchCloudSkillsJsonhas noAbortController/timeout, so a stalled cloud endpoint can leavepublishSkillInventory/getAccountSkillInventories(and the Skills UI busy state) hanging indefinitely. A bounded timeout would degrade gracefully to local inventory, matching the fallback intent described in the PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/src/lib/skills-api.ts` around lines 131 - 136, Update fetchCloudSkillsJson to use an AbortController with a bounded timeout when issuing the cloud fetch, ensuring the timer is cleaned up after completion or failure. Propagate timeout failures through the existing fallback handling so publishSkillInventory and getAccountSkillInventories can degrade to local inventory instead of remaining pending.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dashboard/src/lib/skills-inventory.ts`:
- Around line 118-120: Update the existing-skill merge logic around
addDeviceSource so it preserves existing.targets exactly and does not union in
remote.targets; only append the device source while retaining remote-only target
state separately where applicable.
In `@dashboard/src/pages/SkillsPage.jsx`:
- Around line 836-854: The loadInstalled auth guard must invalidate in-flight
cloud inventory requests when authentication or cloud sync changes. Increment
cloudInventoryRequest before the guard, and when signedIn, cloud sync, deviceId,
or getAccessToken is unavailable, clear cloud-derived inventory rows and device
metadata before returning; preserve the existing requestId-based stale-response
check for valid cloud loads.
---
Outside diff comments:
In `@dashboard/src/pages/SkillsPage.jsx`:
- Around line 1132-1140: Use the stable identity skill.id || skill.directory
throughout selection and busy-state handling. In
dashboard/src/pages/SkillsPage.jsx at lines 1132-1140, 188, 557-558, 1107-1109,
1146-1148, and 1168-1170, update handleToggleSelect, checked state,
targetBusyKey calls, and selectedIds filters to use that fallback identity; make
the corresponding busy-state update in dashboard/src/pages/SkillDetailPanel.jsx
at line 329.
---
Nitpick comments:
In `@dashboard/src/lib/skills-api.ts`:
- Around line 131-136: Update fetchCloudSkillsJson to use an AbortController
with a bounded timeout when issuing the cloud fetch, ensuring the timer is
cleaned up after completion or failure. Propagate timeout failures through the
existing fallback handling so publishSkillInventory and
getAccountSkillInventories can degrade to local inventory instead of remaining
pending.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bf0590e-fe14-40cb-91b2-19056e93ad26
⛔ Files ignored due to path filters (1)
dashboard/src/content/copy.csvis excluded by!**/*.csv
📒 Files selected for processing (18)
dashboard/edge-patches/tokentracker-account-skills.tsdashboard/src/content/i18n/de/core.jsondashboard/src/content/i18n/ja/core.jsondashboard/src/content/i18n/ko/core.jsondashboard/src/content/i18n/zh-TW/core.jsondashboard/src/content/i18n/zh/core.jsondashboard/src/lib/skills-api.tsdashboard/src/lib/skills-inventory.test.tsdashboard/src/lib/skills-inventory.tsdashboard/src/pages/SkillDetailPanel.jsxdashboard/src/pages/SkillsPage.jsxdashboard/src/pages/SkillsPage.test.jsxdashboard/vite.config.jsmigrations/20260721113000_add-device-skill-inventories.sqlsrc/lib/local-api.jssrc/lib/skills-manager.jstest/skills-cloud-guardrails.test.jstest/skills-manager-extras.test.js
| const existing = merged[existingIndex]; | ||
| const targets = [...new Set([...(existing.targets || []), ...(remote.targets || [])])]; | ||
| merged[existingIndex] = addDeviceSource({ ...existing, targets }, source); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Do not mutate the local targets array with remote targets.
Unioning remote.targets into an existing local skill's targets array corrupts the local state. The targets array of an existing local skill acts as the source of truth for the local UI (e.g., the Detail Panel checkboxes) and API mutations (handleToggleTarget, handleBulkSync). If a target is synced on a remote device but not locally, the union falsely marks it as active in the local skill.targets array. Consequently, the Detail Panel will display it as checked, and any subsequent target toggle will unexpectedly sync the remote target to the local device.
Keep existing.targets unchanged for local skills and only append the device source.
🐛 Proposed fix
- const existing = merged[existingIndex];
- const targets = [...new Set([...(existing.targets || []), ...(remote.targets || [])])];
- merged[existingIndex] = addDeviceSource({ ...existing, targets }, source);
+ const existing = merged[existingIndex];
+ merged[existingIndex] = addDeviceSource(existing, source);📝 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.
| const existing = merged[existingIndex]; | |
| const targets = [...new Set([...(existing.targets || []), ...(remote.targets || [])])]; | |
| merged[existingIndex] = addDeviceSource({ ...existing, targets }, source); | |
| const existing = merged[existingIndex]; | |
| merged[existingIndex] = addDeviceSource(existing, source); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard/src/lib/skills-inventory.ts` around lines 118 - 120, Update the
existing-skill merge logic around addDeviceSource so it preserves
existing.targets exactly and does not union in remote.targets; only append the
device source while retaining remote-only target state separately where
applicable.
| const loadInstalled = useCallback(async () => { | ||
| const data = await getInstalledSkills(); | ||
| setInstalledData({ skills: data.skills || [], targets: data.targets || [] }); | ||
| }, []); | ||
| const localData = { skills: data.skills || [], targets: data.targets || [] }; | ||
| setInstalledData(localData); | ||
|
|
||
| const deviceId = getCurrentDeviceId(); | ||
| if (!signedIn || !getCloudSyncEnabled() || !deviceId || typeof getAccessToken !== "function") return; | ||
| const requestId = ++cloudInventoryRequest.current; | ||
| // Cloud inventory is a best-effort enrichment. Local Skills render | ||
| // immediately and remain usable if auth/network/backend sync is unavailable. | ||
| void (async () => { | ||
| try { | ||
| const accessToken = await getAccessToken(); | ||
| if (!accessToken) return; | ||
| const [cloud] = await Promise.all([ | ||
| getAccountSkillInventories(accessToken), | ||
| publishSkillInventory({ accessToken, deviceId, skills: localData.skills }), | ||
| ]); | ||
| if (cloudInventoryRequest.current !== requestId) return; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section and nearby state/effects.
FILE="dashboard/src/pages/SkillsPage.jsx"
wc -l "$FILE"
sed -n '760,920p' "$FILE"
printf '\n--- search: cloudInventoryRequest / setInstalledData / sign-out / cloud sync ---\n'
rg -n "cloudInventoryRequest|setInstalledData|getCloudSyncEnabled|signedIn|getAccessToken|publishSkillInventory|getAccountSkillInventories|setCloud|signOut|sign-in|sign out|cloud sync" "$FILE"Repository: mm7894215/TokenTracker
Length of output: 7198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the specific component region and related state-management helpers.
FILE="dashboard/src/pages/SkillsPage.jsx"
echo "=== file size ==="
wc -l "$FILE"
echo
echo "=== loadInstalled region ==="
sed -n '820,900p' "$FILE"
echo
echo "=== adjacent effect / cleanup regions ==="
sed -n '1,220p' "$FILE" | rg -n "useEffect|useMemo|useCallback|cloudInventoryRequest|setInstalledData|setCloud|signedIn|getCloudSyncEnabled|signOut|auth"
echo
echo "=== all occurrences of cloudInventoryRequest ==="
rg -n "cloudInventoryRequest" "$FILE"Repository: mm7894215/TokenTracker
Length of output: 3625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="dashboard/src/pages/SkillsPage.jsx"
printf '=== loadInstalled callers / effects ===\n'
rg -n "loadInstalled\(|loadInitial\(|signedIn|getCloudSyncEnabled|cloudInventoryRequest|setInstalledData\(" "$FILE"
printf '\n=== effect region around initial loading and auth changes ===\n'
sed -n '900,1100p' "$FILE"
printf '\n=== any state clear paths for installedData ===\n'
rg -n "setInstalledData\(|mergeSkillInventories|clear|reset|sign out|signed out|signedIn" "$FILE"Repository: mm7894215/TokenTracker
Length of output: 8998
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External
● Entry
dashboard/src/pages/SkillsPage.test.jsx
│
▼
● Sink
dashboard/src/pages/SkillsPage.jsx
Invalidate pending cloud inventory on auth changes cloudInventoryRequest is only bumped after the auth/cloud-sync guard, so a request started before sign-out can still merge the previous account’s cloud inventory into installedData. Bump the generation before the guard and clear cloud-derived rows/device metadata when auth or cloud sync turns off so stale enrichment can’t repopulate a signed-out session.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 838-838: Avoid using the initial state variable in setState
Context: setInstalledData(localData)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard/src/pages/SkillsPage.jsx` around lines 836 - 854, The loadInstalled
auth guard must invalidate in-flight cloud inventory requests when
authentication or cloud sync changes. Increment cloudInventoryRequest before the
guard, and when signedIn, cloud sync, deviceId, or getAccessToken is
unavailable, clear cloud-derived inventory rows and device metadata before
returning; preserve the existing requestId-based stale-response check for valid
cloud loads.
1 participant
Summary
Privacy and safety
Only stable keys, display names, relative directories, tool IDs, scope flags, and device metadata are synced.
SKILL.mdcontent, descriptions, prompts, README URLs, target paths, and absolute paths are excluded. Both the browser projection and Edge function reject absolute/path-bearing identifiers. Remote, built-in, and plugin-managed rows cannot be deleted, batch-selected, or retargeted.Deployment required
migrations/20260721113000_add-device-skill-inventories.sqltokentracker-account-skillsEdge function fromdashboard/edge-patches/tokentracker-account-skills.tsCross-device inventory respects the existing cloud-sync preference and degrades to the local inventory if auth, network, or backend sync is unavailable.
Validation
validate:copy,validate:ui-hardcode, andvalidate:guardrailsgit diff --checkThe full root
npm run ci:localdoes not run cleanly on this Windows host because of existing environment-specific failures (missingsqlite3, symlinkEPERM, and path/locale assumptions); the scoped Node suites and the complete Dashboard suite pass.Summary by CodeRabbit
New Features
Bug Fixes
Tests