Mcp skills#571
Conversation
| @@ -0,0 +1,178 @@ | |||
| /* | |||
There was a problem hiding this comment.
Missing tests for skills.ts
All existing MCP tools have unit tests in spec/unit/McpToolsSpec.js. This new module has no test coverage.
Suggested approach:
- Unit tests with mocked
fetchto cover: manifest parsing, skill lookup, error handling for unknown skills, network failure graceful degradation, and caching behavior. - Optionally a single integration smoke test (skippable without network) that validates the real remote
skills.jsonis parseable. - Export a cache reset helper (e.g.
_resetCache()) to isolate tests from each other.
| const REPO_NAME = "powerbi-visuals-skills"; | ||
| const BRANCH = "main"; | ||
| const RAW_BASE = `https://raw.githubusercontent.com/${REPO_OWNER}/${REPO_NAME}/${BRANCH}`; | ||
| const API_BASE = `https://api.github.com/repos/${REPO_OWNER}/${REPO_NAME}/contents`; |
There was a problem hiding this comment.
Rate limiting risk: consider removing api.github.com dependency
api.github.com has a strict 60 req/hr limit for unauthenticated requests. fetchDirListing + collectFilesRecursively can consume multiple requests per single implement_feature call.
Suggested solution: in the root skills.json manifest, extend each skill descriptor with a files array listing all file paths relative to the skill directory. Example:
json { "id": "bookmarks", "path": "skills/bookmarks", "files": ["SKILL.md", "references/capabilities.json"] }
Then fetch all content exclusively via raw.githubusercontent.com using known paths — no directory listing needed. This eliminates the api.github.com dependency entirely.
Don't forget to update the related documentation in the powerbi-visuals-skills repository (README, CONTRIBUTING, schema description) if adding the files field.
If keeping api.github.com short-term: at minimum, catch 403/429 responses and show a user-friendly message with reset time from X-RateLimit-Reset header.
|
|
||
| let cachedManifest: SkillsManifest | null = null; | ||
|
|
||
| async function fetchRawFile(repoPath: string): Promise<string> { |
There was a problem hiding this comment.
Manifest cache lives forever (no TTL)
The MCP server process lives as long as the IDE session (STDIO transport, killed only on disconnect/SIGINT). This can be hours or days. cachedManifest is never invalidated, so newly published skills won't appear until the user restarts the IDE.
Suggestion: add a TTL (e.g. 30-60 min) — store a timestamp alongside the cached value and re-fetch if expired.
| } | ||
| } | ||
| } catch { | ||
| // If listing fails (e.g., rate limit), return what we have |
There was a problem hiding this comment.
Silent error swallowing — user gets incomplete results without notice
If fetching additional skill files fails, the error is silently discarded. The user receives a partial response with no indication that something is missing.
Regardless of the fetch mechanism used, when individual files fail to load: include a warning in the response (e.g. "warnings": ["Failed to fetch references/capabilities.json"]) so the AI agent and user are aware the response is incomplete.
| version: "1.0.0", | ||
| }, | ||
| { | ||
| instructions: |
There was a problem hiding this comment.
Overloaded server instructions — consider a concise semantic formulation
LLMs should understand intent semantically — they don't do keyword matching. Listing every possible phrase and spelling variation ("bookmrks", "toltips") likely doesn't improve tool selection — it dilutes the signal and inflates token usage on every exchange.
Consider replacing the 30+ line instructions with a short role-based formulation, e.g.:
"MCP server for Power BI custom visual development. Provides tools for: implementing visual features, checking best practices and security, certification readiness, project info, and available APIs. Use these tools whenever the user's request relates to Power BI visual development."
The tool selection guide (which tool for which intent) is better conveyed by each tool's own description.
| "get_best_practices", | ||
| "Returns best practice guidelines for Power BI custom visual development. Covers: API version management, performance optimization (update loop, lazy loading, data processing), security (eval, innerHTML, XSS, sanitization, external call/calls, network request/requests), accessibility (keyboard navigation, high contrast, screen reader, ARIA label/labels), project structure (module/modules, error handling), formatting pane (format model, formatting model), testing (unit test/tests, E2E test/tests, edge case/cases), and documentation (README, changelog, comment/comments).", | ||
| "MUST CALL when user asks about Power BI visual quality, standards, or how to build correctly. " + | ||
| "Get best practices, coding guidelines, and recommendations for building Power BI custom visuals. " + |
There was a problem hiding this comment.
Tool descriptions: prefer semantic clarity over keyword lists
"MUST CALL", "Trigger phrases: ..." and keyword enumerations likely don't help LLMs — they should understand natural language intent. A clear, concise description of what the tool does and when to use it might work better and save tokens.
Example — instead of the current 6-line description, something like:
"Get best practices and coding guidelines for Power BI custom visual development. Covers performance, security, accessibility, project structure, and testing."
Apply the same principle to all tool descriptions.
| "total, subtotal, sub-total, allow interactions, interactions, fetch more data, color palette. " + | ||
| "Also call when user says: 'support X', 'implement X', 'enable X', 'how to add X', 'I need X', 'integrate X'. " + | ||
| "Even with typos like 'bookmrks', 'toltips', 'contex menu' — STILL call this tool. " + | ||
| "Returns feature IDs — then call get_feature_documentation with the ID for full instructions.", |
There was a problem hiding this comment.
Bug: references non-existent tool get_feature_documentation. Should be implement_feature.
| const available = manifest.skills.map(s => s.id); | ||
| return JSON.stringify({ | ||
| error: `Feature "${skillName}" not found.`, | ||
| message: `Please pick a valid feature ID from the list below and call get_feature_documentation again.`, |
There was a problem hiding this comment.
Bug: references non-existent tool get_feature_documentation. Should be implement_feature.
| | `prepare_certification` | Check if the visual is ready for certification and marketplace submission | ✅ | ✅ | | ||
| | `list_visual_info` | Get info about current visual (name, GUID, API version, capabilities) | ✅ | ✅ | | ||
| | `get_available_apis` | List available Power BI Visual APIs with examples | ✅ | ✅ | | ||
| | `add_feature` | List all features that can be added to a Power BI visual | ✅ | ✅ | |
There was a problem hiding this comment.
add_feature and implement_feature fetch data from a remote GitHub repository. The "Local" column should be ❌ for these tools (at minimum add_feature requires a remote request at least once per session to fetch the manifest). They don't send any user data externally, but they do require network access to function.
2 participants
No description provided.