feat: show and sync complete Skills inventories - #349
Conversation
📝 WalkthroughWalkthroughAdds bounded local plugin inventory discovery, authenticated per-device cloud synchronization, sanitized cross-device merging, and dashboard handling for remote or read-only skills. ChangesSkills inventory synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SkillsPage
participant skillsInventory
participant skillsApi
participant tokentrackerAccountSkills
participant DeviceInventoryTable
SkillsPage->>skillsInventory: buildSkillInventoryMetadata(localSkills)
SkillsPage->>skillsApi: publishSkillInventory(accessToken, deviceId, skills)
skillsApi->>tokentrackerAccountSkills: POST sanitized device inventory
tokentrackerAccountSkills->>DeviceInventoryTable: upsert user/device skills
SkillsPage->>skillsApi: getAccountSkillInventories(accessToken)
skillsApi->>tokentrackerAccountSkills: GET account inventories
tokentrackerAccountSkills->>DeviceInventoryTable: query active devices and inventories
DeviceInventoryTable-->>tokentrackerAccountSkills: device inventory rows
tokentrackerAccountSkills-->>skillsApi: devices payload
skillsApi-->>SkillsPage: cloud inventories
SkillsPage->>skillsInventory: mergeSkillInventories(localSkills, cloudPayload, deviceId)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 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
|
Thanks for working on the complete Skills inventory and cross-device sync! Overall this expands Skill visibility nicely. Before merging, we have a couple of optimization points to address:
Please update the PR with these adjustments. Thank you! |
6d4c36d to
16d32db
Compare
|
Updated in
Rebased onto the latest |
xiufengsun
left a comment
xiufengsun
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-reviewed the exact head (16d32db) on top of the latest main. The existing GitHub checks are green, but two merge blockers remain:
-
The latest main now runs the locale coverage gate. This PR adds seven product UI strings under skills.inventory.* without locale entries, so npm run validate:locale fails for zh and zh-TW. Skill names and SKILL.md metadata should remain in their original form, but surrounding UI copy such as “Plugin”, “Other device”, and the read-only/device labels still needs localization. Please add translations for those seven UI keys without translating Skill-defined names or metadata.
-
mergeSkillInventories() loses target information when the same Skill exists only on multiple remote devices with different targets. Once the first remote-only row is created, the existingIndex branch only appends the next device source and leaves the first device's targets/targetStates unchanged. For example, Claude on one remote device plus Codex on another renders only Claude while showing both device badges. Please preserve local targets for local matches, but merge target state for an existing remote-only row (or retain per-device target state), and add a regression test with different targets on two remote devices.
Validation on latest main + this PR otherwise looks good: 1,756/1,756 Node tests passed after building dashboard/dist, the Skills-focused Node and Dashboard tests passed, and typecheck, ESLint, production build, copy/UI/architecture guardrails all passed.
After these two issues are fixed, please also ensure the migration and tokentracker-account-skills Edge function are deployed before release; neither backend object is currently present in the linked InsForge project.
16d32db to
5bac50e
Compare
|
Updated in
The new head is mergeable and all GitHub checks pass: Linux test/validate/build, Windows build, macOS unit tests, CodeQL, and labeling. The migration and |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/edge-patches/tokentracker-account-skills.ts`:
- Around line 49-76: Update verifiedUserIdFromJwt to require payload.exp to be a
finite number and reject the token when exp is missing, invalid, or expired
before accepting payload.sub or payload.user_id. Preserve the existing
expiration comparison and identity fallback behavior for valid, unexpired
tokens.
🪄 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: 47b12566-4366-4fe1-91f6-182a754fec5b
⛔ Files ignored due to path filters (1)
dashboard/src/content/copy.csvis excluded by!**/*.csv
📒 Files selected for processing (15)
dashboard/edge-patches/tokentracker-account-skills.tsdashboard/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
🚧 Files skipped from review as they are similar to previous changes (6)
- migrations/20260721113000_add-device-skill-inventories.sql
- src/lib/local-api.js
- dashboard/src/pages/SkillsPage.test.jsx
- dashboard/src/lib/skills-inventory.test.ts
- dashboard/src/lib/skills-api.ts
- dashboard/src/lib/skills-inventory.ts
| async function verifiedUserIdFromJwt(authHeader: string | null): Promise<string | null> { | ||
| if (!authHeader) return null; | ||
| const token = authHeader.replace(/^Bearer\s+/i, "").trim(); | ||
| const secret = Deno.env.get("JWT_SECRET"); | ||
| const parts = token.split("."); | ||
| if (!token || !secret || parts.length !== 3) return null; | ||
| try { | ||
| const key = await crypto.subtle.importKey( | ||
| "raw", | ||
| new TextEncoder().encode(secret), | ||
| { name: "HMAC", hash: "SHA-256" }, | ||
| false, | ||
| ["verify"], | ||
| ); | ||
| const ok = await crypto.subtle.verify( | ||
| "HMAC", | ||
| key, | ||
| b64urlToBytes(parts[2]), | ||
| new TextEncoder().encode(`${parts[0]}.${parts[1]}`), | ||
| ); | ||
| if (!ok) return null; | ||
| const payload = JSON.parse(new TextDecoder().decode(b64urlToBytes(parts[1]))) as Record<string, unknown>; | ||
| if (typeof payload.exp === "number" && Date.now() / 1000 > payload.exp) return null; | ||
| if (typeof payload.sub === "string" && payload.sub) return payload.sub; | ||
| if (typeof payload.user_id === "string" && payload.user_id) return payload.user_id; | ||
| } catch { /* invalid token */ } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- Target file excerpt ---\n'
if [ -f dashboard/edge-patches/tokentracker-account-skills.ts ]; then
nl -ba dashboard/edge-patches/tokentracker-account-skills.ts | sed -n '1,140p'
else
printf 'Target file not found\n'
fi
printf '\n--- Repository JWT/validation references ---\n'
rg -n "(verifiedUserIdFromJwt|JWT_SECRET|access_token|payload\.exp|exp|aud|sub|user_id|auth\.users|insert\(\[)" -S --glob '!node_modules' --glob '!dist' --glob '!build' . | head -n 200
printf '\n--- Edge pricing parity test references if present ---\n'
if [ -f dashboard/test/edge-pricing-parity.test.js ]; then
wc -l dashboard/test/edge-pricing-parity.test.js
rg -n "tokentracker-account-skills|price|exp|JWT_SECRET|token" dashboard/test/edge-pricing-parity.test.js -S || true
fiRepository: mm7894215/TokenTracker
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- Target file excerpt ---'
if [ -f dashboard/edge-patches/tokentracker-account-skills.ts ]; then
nl -ba dashboard/edge-patches/tokentracker-account-skills.ts | sed -n '1,140p'
else
printf '%s\n' 'Target file not found'
fi
printf '\n%s\n' '--- Repository JWT/validation references ---'
rg -n "verifiedUserIdFromJwt|JWT_SECRET|access_token|payload\.exp|\.exp|exp|aud|sub|user_id|auth\.users|insert\(\[" -S . | head -n 200 || true
printf '\n%s\n' '--- Edge pricing parity test references if present ---'
if [ -f dashboard/test/edge-pricing-parity.test.js ]; then
wc -l dashboard/test/edge-pricing-parity.test.js
rg -n "tokentracker-account-skills|price|exp|JWT_SECRET|token" dashboard/test/edge-pricing-parity.test.js || true
fiRepository: mm7894215/TokenTracker
Length of output: 25474
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- Target file line count and relevant excerpt ---'
if [ -f dashboard/edge-patches/tokentracker-account-skills.ts ]; then
wc -l dashboard/edge-patches/tokentracker-account-skills.ts
sed -n '1,160p' dashboard/edge-patches/tokentracker-account-skills.ts | cat -n
else
printf '%s\n' 'Target file not found'
fi
printf '\n%s\n' '--- Targeted JWT/validation matches ---'
grep -EIn "verifiedUserIdFromJwt|JWT_SECRET|payload\.exp|exp|sub|user_id|insert\\(\\[|onConflict" dashboard/edge-patches/tokentracker-account-skills.ts || true
printf '\n%s\n' '--- Edge pricing parity test file ---'
if [ -f dashboard/test/edge-pricing-parity.test.js ]; then
wc -l dashboard/test/edge-pricing-parity.test.js
sed -n '1,260p' dashboard/test/edge-pricing-parity.test.js | cat -n
else
printf '%s\n' 'dashboard/test/edge-pricing-parity.test.js not found'
fi
printf '\n%s\n' '--- Edge patch directory files ---'
ls -1 dashboard/edge-patches/*.ts 2>/dev/null || trueRepository: mm7894215/TokenTracker
Length of output: 8644
Broken Authentication (CWE-613): Insufficient Session Expiration
Reachability: External
● Entry
dashboard/src/pages/SkillsPage.jsx
│
▼
● Hop
dashboard/src/lib/skills-inventory.ts:31
cleanRelativePath
│
▼
● Sink
dashboard/edge-patches/tokentracker-account-skills.ts
Require JWT exp validation instead accepting a token without it.
verifiedUserIdFromJwt only expires the JWT when payload.exp is a number; otherwise it returns on sub/user_id. A token without exp is therefore treated as never-expiring, so require exp to be present and valid before authorizing the endpoint.
🔒 Proposed fix
- if (typeof payload.exp === "number" && Date.now() / 1000 > payload.exp) return null;
+ if (typeof payload.exp !== "number" || Date.now() / 1000 > payload.exp) return null;Whether this is defense-in-depth or an exploit gap depends on whether InsForge-issued JWTs are guaranteed to include exp; if not, reject tokens missing it before returning the user identity.
📝 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.
| async function verifiedUserIdFromJwt(authHeader: string | null): Promise<string | null> { | |
| if (!authHeader) return null; | |
| const token = authHeader.replace(/^Bearer\s+/i, "").trim(); | |
| const secret = Deno.env.get("JWT_SECRET"); | |
| const parts = token.split("."); | |
| if (!token || !secret || parts.length !== 3) return null; | |
| try { | |
| const key = await crypto.subtle.importKey( | |
| "raw", | |
| new TextEncoder().encode(secret), | |
| { name: "HMAC", hash: "SHA-256" }, | |
| false, | |
| ["verify"], | |
| ); | |
| const ok = await crypto.subtle.verify( | |
| "HMAC", | |
| key, | |
| b64urlToBytes(parts[2]), | |
| new TextEncoder().encode(`${parts[0]}.${parts[1]}`), | |
| ); | |
| if (!ok) return null; | |
| const payload = JSON.parse(new TextDecoder().decode(b64urlToBytes(parts[1]))) as Record<string, unknown>; | |
| if (typeof payload.exp === "number" && Date.now() / 1000 > payload.exp) return null; | |
| if (typeof payload.sub === "string" && payload.sub) return payload.sub; | |
| if (typeof payload.user_id === "string" && payload.user_id) return payload.user_id; | |
| } catch { /* invalid token */ } | |
| return null; | |
| } | |
| async function verifiedUserIdFromJwt(authHeader: string | null): Promise<string | null> { | |
| if (!authHeader) return null; | |
| const token = authHeader.replace(/^Bearer\s+/i, "").trim(); | |
| const secret = Deno.env.get("JWT_SECRET"); | |
| const parts = token.split("."); | |
| if (!token || !secret || parts.length !== 3) return null; | |
| try { | |
| const key = await crypto.subtle.importKey( | |
| "raw", | |
| new TextEncoder().encode(secret), | |
| { name: "HMAC", hash: "SHA-256" }, | |
| false, | |
| ["verify"], | |
| ); | |
| const ok = await crypto.subtle.verify( | |
| "HMAC", | |
| key, | |
| b64urlToBytes(parts[2]), | |
| new TextEncoder().encode(`${parts[0]}.${parts[1]}`), | |
| ); | |
| if (!ok) return null; | |
| const payload = JSON.parse(new TextDecoder().decode(b64urlToBytes(parts[1]))) as Record<string, unknown>; | |
| if (typeof payload.exp !== "number" || Date.now() / 1000 > payload.exp) return null; | |
| if (typeof payload.sub === "string" && payload.sub) return payload.sub; | |
| if (typeof payload.user_id === "string" && payload.user_id) return payload.user_id; | |
| } catch { /* invalid token */ } | |
| return null; | |
| } |
🤖 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/edge-patches/tokentracker-account-skills.ts` around lines 49 - 76,
Update verifiedUserIdFromJwt to require payload.exp to be a finite number and
reject the token when exp is missing, invalid, or expired before accepting
payload.sub or payload.user_id. Preserve the existing expiration comparison and
identity fallback behavior for valid, unexpired tokens.
|
Thanks for the updates — the privacy boundary and read-only handling look solid now. Two things to fix before merge:
Minor, fine as follow-ups: inventory rows for revoked devices are never cleaned up, and every mutation triggers a full GET+POST round trip (a projection-hash skip would help). |
2 participants
Summary
Review updates
SKILL.md; the added locale dictionary changes have been removedid || directoryidentity for unmanaged Skill selection, list keys, and busy statesPrivacy 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 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 --checkSummary by CodeRabbit