Skip to content

feat: MCP Server, Knowledge System, and Skills for Player UI#812

Open
rafbcampos wants to merge 12 commits intomainfrom
mcp
Open

feat: MCP Server, Knowledge System, and Skills for Player UI#812
rafbcampos wants to merge 12 commits intomainfrom
mcp

Conversation

@rafbcampos
Copy link
Contributor

@rafbcampos rafbcampos commented Mar 10, 2026

Overview

This PR introduces an AI-assisted development toolkit for Player UI, consisting of three systems:

  1. Knowledge artifacts for core packages (@player-ui/types, @player-ui/player, @player-ui/react, JVM core)
  2. MCP (Model Context Protocol) server for on-demand knowledge retrieval
  3. Skills — curated, installable Claude Code skills for common Player UI development tasks

Motivation

Traditional documentation is written for humans, not LLMs. LLMs need a different approach:

  • Context efficiency: LLMs have token limits; only inject what's needed
  • Concept-focused: Emphasize "why" and "when" over exhaustive API listings
  • Pattern-based: Describe common tasks as reusable patterns
  • Dependency-aware: Automatically include related packages
  • Searchable: Find packages by concept (e.g., "validation", "data binding")
  • Task-guided: Skills provide step-by-step workflows for plugin creation across all platforms

This enables tools like Claude Code to assist with Player UI without exceeding context limits.

What's Changed

📚 Knowledge System (tools/mcp-server/knowledge/)

knowledge/
├── README.md              # Documentation for contributors
├── metadata.json          # Package catalog with dependencies & search index
├── core/
│   ├── types.md           # @player-ui/types (~1,800 tokens)
│   └── player.md          # @player-ui/player (~2,600 tokens)
├── react/
│   └── player.md          # @player-ui/react (~2,750 tokens)
└── jvm/
    └── core.md            # JVM/Kotlin core (~2,500 tokens)

Knowledge Artifact Features:

  • Concise (1,500-3,000 tokens per package)
  • Consistent structure: Overview → Concepts → APIs → Patterns → Pitfalls
  • Common usage patterns with "when to use" guidance
  • Integration points and cross-package relationships
  • Common pitfalls from real-world usage

🔧 MCP Server (tools/mcp-server/)

A Model Context Protocol server that exposes four tools:

1. player_get_package

Retrieve detailed knowledge for a package with automatic dependency resolution.

// Example: Get knowledge for React package
player_get_package({
  package: "@player-ui/react",
  includeDependencies: true, // Includes player + types
});

Returns: Markdown with concepts, APIs, patterns, and pitfalls.

2. player_search_api

Search for packages by concept, keyword, or API name.

// Example: Find validation-related packages
player_search_api({
  query: "validation",
  scope: "core",
});

Returns: Matching packages with relevance type.

3. player_detect_packages

Auto-detect Player UI packages from package.json and provide quick reference.

// Example: Scan current project
player_detect_packages({
  packageJsonPath: "package.json",
});

Returns: Detected packages with overview of each.

4. player_list_packages

List all available Player UI packages with descriptions and metadata.

// Example: List all packages
player_list_packages({
  category: "all",
});

// Or filter by category
player_list_packages({
  category: "core",
});

Returns: List of packages grouped by category with tags, exports, dependencies, and token counts.

Internal Modules (tools/mcp-server/src/)

  • loader.ts — reads knowledge artifacts from disk, resolves dependencies recursively
  • cache.ts — LRU cache for loaded knowledge artifacts
  • config.ts — environment-based configuration (MCP_MAX_DEPTH, MCP_CACHE_MAX_SIZE, etc.)
  • validation.ts — input validation for all tool arguments
  • errors.ts — typed error classes (e.g., PathTraversalError, PackageNotFoundError)
  • logger.ts — structured logging to stderr
  • detect-packages.ts — utility to scan package.json for Player UI dependencies

🛠️ Skills (tools/skills/)

A Claude Code skills installer (@player-ui/skills) that bundles curated skill files for Player UI development. Skills are self-contained markdown guides with YAML front matter that walk AI assistants through common tasks.

CLI Commands:

  • player-ui-skills install — Interactively select and install skills to local (.claude/skills/) or global (~/.claude/skills/) scope
  • player-ui-skills list — View and manage currently installed skills

Available Skills (7):

Skill Description
create-core-plugin Create TypeScript plugins for the core Player runtime (PlayerPlugin interface, hooks, expressions, schema/validation, data middleware)
create-react-plugin Create React plugins (ReactPlayerPlugin interface, asset registration, ReactPlayer hooks, context providers)
create-android-plugin Create Android/Kotlin plugins (AndroidPlayerPlugin, JVM hooks, JSScriptPluginWrapper, lifecycle)
create-jvm-plugin Create JVM/Kotlin plugins (PlayerPlugin, JSScriptPluginWrapper, RuntimePlugin, LoggerPlugin, coroutine integration)
create-swift-plugin Create Swift/iOS plugins (NativePlugin, JSBasePlugin, SwiftUI hooks, ManagedPlayerPlugin)
create-multiplatform-plugin Create plugins spanning TypeScript, React, Android, iOS, and SwiftUI (architecture, shared concepts, native IIFE bundles)
player-hooks-guide Reference guide for the Player UI hook system (all hook surfaces, tapable-ts patterns, controller hooks)

Tests

MCP Server: Unit tests for every module (cache, config, errors, logger, resolver, validation, index) and each tool handler (detect-packages, get-package, list-packages, search-api), plus an e2e consumer simulation test. 12 test files total.

Skills: Unit tests for CLI commands (install, list) and library modules (installer, paths, skills). 5 test files total.

🎯 Key Features

  1. Context-Efficient: Loads knowledge only for packages in use
  2. Dependency-Aware: Includes transitive dependencies (depth 2)
  3. Search-Enabled: Find packages by concept using keyword search
  4. Caching: LRU cache for knowledge artifacts; metadata loaded once
  5. Security: Path traversal protection, input validation
  6. Type-Safe: TypeScript with type guards, no any
  7. Error Handling: Error messages with suggestions
  8. Cross-Platform Skills: Guided workflows for plugin creation on all supported platforms (TS, React, Android, JVM, Swift)

Architecture

Progressive Disclosure

The system uses three-tiered knowledge delivery:

Tier 1: Auto-Detection (Zero-effort)

  • MCP server scans package.json on project open
  • Identifies Player UI packages automatically
  • Provides quick reference without user action

Tier 2: On-Demand (Low-effort)

  • User asks question mentioning package name
  • Claude calls player_get_package automatically
  • Retrieves full knowledge + dependencies

Tier 3: Search (Exploratory)

  • User searches for capability (e.g., "validation")
  • Claude calls player_search_api
  • Returns relevant packages to explore

Skills Workflow

Skills complement the knowledge system by providing task-level guidance:

  • Knowledge tells the LLM what Player UI APIs exist and how they work
  • Skills tell the LLM how to accomplish specific tasks step-by-step

Users install skills via the CLI, and Claude Code automatically picks them up from .claude/skills/.

Dependency Resolution

When retrieving a package, the system automatically includes:

@player-ui/react (requested)
  ├─ @player-ui/player (direct dependency, depth 1)
  │   └─ @player-ui/types (transitive, depth 2)
  └─ @player-ui/types (direct dependency, depth 1)

Usage

For Claude Code Users

MCP Server — Add to .claude/config.json:

{
  "mcpServers": {
    "player-ui": {
      "command": "npx",
      "args": ["@player-ui/mcp-server"]
    }
  }
}

Skills — Install via CLI:

npx @player-ui/skills install

Then ask Claude questions about Player UI — the knowledge system is queried as needed and skills guide plugin creation.

Build System Integration

Both packages are integrated with the Bazel monorepo:

# Build the MCP server
bazel build //tools/mcp-server:mcp-server_tsup

# Build the skills CLI
bazel build //tools/skills:skills_tsup

Dependencies Added

  • @modelcontextprotocol/sdk: ^1.0.0 — added to root package.json; referenced in the BUILD file via //:node_modules/@modelcontextprotocol/sdk

Other Changes

  • .bazelignore updated to exclude tools/mcp-server/node_modules and tools/skills/node_modules

Breaking Changes

None. This is a purely additive feature that doesn't affect existing code.

@rafbcampos rafbcampos self-assigned this Mar 10, 2026
@rafbcampos rafbcampos added the minor Increment the minor version when merged label Mar 10, 2026
@rafbcampos
Copy link
Contributor Author

/canary

@codecov
Copy link

codecov bot commented Mar 10, 2026

Bundle Report

Changes will increase total bundle size by 118.53kB (2.11%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
tools/mcp-server 94.09kB 94.09kB (100%) ⬆️
tools/skills 24.45kB 24.45kB (100%) ⬆️

@codecov
Copy link

codecov bot commented Mar 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.87%. Comparing base (0d321b8) to head (7e7e439).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #812   +/-   ##
=======================================
  Coverage   85.87%   85.87%           
=======================================
  Files         505      505           
  Lines       22894    22894           
  Branches     2660     2660           
=======================================
  Hits        19661    19661           
  Misses       2906     2906           
  Partials      327      327           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@intuit-svc
Copy link
Contributor

Build Preview

Your PR was deployed by CircleCI #32412 on Tue, 10 Mar 2026 14:32:08 GMT with this version:

0.16.0--canary.812.32412

📖 Docs (View site)

@rafbcampos rafbcampos marked this pull request as ready for review March 12, 2026 14:32
@rafbcampos rafbcampos requested review from a team as code owners March 12, 2026 14:32
@rafbcampos rafbcampos requested a review from a team as a code owner March 16, 2026 19:31
@spentacular
Copy link
Contributor

@claude can you review this PR?

@claude
Copy link

claude bot commented Mar 17, 2026

Claude finished @spentacular's task in 2m 6s —— View job


PR Review: feat: MCP Server and Knowledge System for Player UI

Tasks:

  • Read CLAUDE.md for repo guidelines
  • Read core source files (loader, cache, validation, errors, index)
  • Read tool handlers
  • Read tests
  • Provide comprehensive review feedback

Overall Assessment

This is a well-crafted PR — clean architecture, thorough test coverage (100% lines covered per Codecov), good security practices, and a useful contribution. Below are findings ranging from bugs to style suggestions.


Bugs / Correctness Issues

1. LRUCache.has() has an unintended side effecttools/mcp-server/src/cache.ts:42

has(key) calls get(key) internally, which promotes the key to MRU position. Callers who want to check existence without affecting eviction order (e.g. debug/stats code, future consumers) will get surprising behavior. The method is currently only called from within cache tests, but it's a public API that could be misused. A safer implementation just delegates to the internal Map.has():

has(key: string): boolean {
  return this.cache.has(key);
}

Fix this →


2. Overview truncation always appends "..." even when text isn't truncatedtools/mcp-server/src/tools/detect-packages.ts:93

// Current — appends "..." unconditionally
const overview = overviewMatch
  ? overviewMatch[1].substring(0, config.overviewMaxLength).trim() + "..."
  : "No overview available.";

If the overview text is shorter than overviewMaxLength, substring() returns the full string and "..." is still appended — misleading the LLM into thinking the content was cut off. Should only append "..." when truncation actually occurred:

const fullOverview = overviewMatch[1].trim();
const overview = fullOverview.length > config.overviewMaxLength
  ? fullOverview.substring(0, config.overviewMaxLength) + "..."
  : fullOverview;

Fix this →


3. parseInt of env vars can return NaNtools/mcp-server/src/config.ts:22-24

maxDependencyDepth: parseInt(process.env.MCP_MAX_DEPTH || "2", 10),

If MCP_MAX_DEPTH=abc is set, parseInt silently returns NaN. This could cause the dependency loader to behave unexpectedly (e.g. currentDepth > NaN is always false, loading unlimited depth). Consider:

const rawDepth = parseInt(process.env.MCP_MAX_DEPTH || "2", 10);
maxDependencyDepth: Number.isNaN(rawDepth) ? 2 : rawDepth,

Same applies to MCP_OVERVIEW_LENGTH and MCP_CACHE_MAX_SIZE.

Fix this →


Design / Code Quality

4. Redundant path traversal conditiontools/mcp-server/src/tools/detect-packages.ts:38

if (
  relativePath.startsWith("..") ||
  resolve(baseDir, relativePath) !== resolvedPath  // ← always false when first check passes
) {

relative(baseDir, resolvedPath) gives a canonical relative path, so resolve(baseDir, relative(baseDir, resolvedPath)) is mathematically the same as resolvedPath. The second operand of || is always false when the paths are valid, making it dead code. The first check (startsWith("..")) is the meaningful one.

Also, the second check at line 44 (!resolvedPath.startsWith(baseDir)) could have a false positive on paths like /foo/bar-evil/ when baseDir is /foo/bar. Prefer resolvedPath.startsWith(baseDir + path.sep) || resolvedPath === baseDir.


5. Double iteration over knowledgeMap in get-package.tstools/mcp-server/src/tools/get-package.ts:49-63

The response is built by iterating the map twice (first for the main package, then for deps). Since Map preserves insertion order and loadRecursive always inserts the root package first, a single pass with a conditional would work and be cleaner:

for (const [pkg, knowledge] of knowledgeMap.entries()) {
  if (pkg !== packageName) {
    response += `---\n\n# Dependency: ${pkg}\n\n`;
  }
  response += knowledge + "\n\n";
}

6. Accessing .server internal property on McpServertools/mcp-server/src/index.ts:131

server.server.setRequestHandler(ListToolsRequestSchema, async () => { ... });

server.server accesses an internal property of McpServer. The MCP SDK's high-level API (server.tool(...)) is the intended registration surface and would be more resilient to SDK upgrades. If the current SDK version forces this pattern (e.g. to support dynamic enum in the schema), a comment explaining why would be valuable.


Test Observations

7. index.test.ts tests mostly exercise mocks, not actual routingtools/mcp-server/__tests__/unit/index.test.ts

Most test cases in this file call the mocked functions directly and verify the mocked return values — they don't exercise the switch statement in the CallToolRequestSchema handler. For example, the "should handle tool execution errors" test at line 370 calls getPackage directly rather than routing through the server. The e2e consumer simulation test likely covers this better, but it's worth noting that the unit test for index.ts has limited coverage of the actual dispatch logic.


Minor Suggestions

  • package.json missing engines field: No minimum Node.js version is specified. Since this uses import.meta.url and ES modules, at minimum Node 18+ should be declared.
  • metadataCache race condition (loader.ts:65): In async contexts where loadMetadata() is called concurrently before the first call resolves, multiple reads of metadata.json will happen. Low impact since Node is single-threaded and the file read is fast, but a promise-based cache (store the in-flight promise rather than the result) would be strictly correct.
  • Relevance ordering (loader.ts:277): export-match ranks higher than tag-match. Tags seem more semantically intentional for search purposes — consider swapping their priority.

Summary

The PR is in good shape and ready for merge after addressing the bugs above (items 1–3 are most impactful). The architecture is sound, error handling is comprehensive, and the test coverage is solid. Great work on the path traversal protection and the structured error hierarchy.

Copy link
Contributor

@spentacular spentacular left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, running through it again tomorrow.

@rafbcampos rafbcampos changed the title feat: MCP Server and Knowledge System for Player UI feat: MCP Server, Knowledge System, and Skills for Player UI Mar 25, 2026
@rafbcampos
Copy link
Contributor Author

/canary

@rafbcampos
Copy link
Contributor Author

@claude can you review this PR?

@claude
Copy link

claude bot commented Mar 25, 2026

Claude finished @rafbcampos's task in 1m 34s —— View job


Claude finished @rafbcampos's task


This PR adds a clean, well-structured AI toolkit for Player UI — MCP server, knowledge system, and skills CLI. Test coverage is thorough and the security model (path traversal protection, typed errors, input validation) is solid. Three bugs from the prior review are still outstanding, plus one new finding:

Unresolved bugs

1. LRUCache.has() promotes key to MRUcache.ts:42

has() calls get(), which moves the key to most-recently-used position. Callers checking existence without intent to access the value will silently corrupt eviction order:

// fix: delegate to Map directly
has(key: string): boolean {
  return this.cache.has(key);
}

2. "..." always appended in overview, even when not truncateddetect-packages.ts:92-95

// current: misleads LLM that content was cut off
overviewMatch[1].substring(0, config.overviewMaxLength).trim() + "..."

// fix: only append when actually truncated
const full = overviewMatch[1].trim();
full.length > config.overviewMaxLength
  ? full.substring(0, config.overviewMaxLength) + "..."
  : full

3. parseInt of env vars can return NaNconfig.ts:22-24

MCP_MAX_DEPTH=abc makes parseInt return NaN. currentDepth > NaN is always false, so dependencies load without bound. Same applies to MCP_OVERVIEW_LENGTH and MCP_CACHE_MAX_SIZE:

const rawDepth = parseInt(process.env.MCP_MAX_DEPTH || "2", 10);
maxDependencyDepth: Number.isNaN(rawDepth) ? 2 : rawDepth,

New finding

4. version should use __VERSION__ substitutionindex.ts:23

const version = "0.0.0-PLACEHOLDER";  // never replaced at build time

The root tsup.config.ts defines __VERSION__ as a build-time substitution for {STABLE_VERSION}. The "0.0.0-PLACEHOLDER" string will be served as-is to MCP clients. Other packages in the repo declare and use __VERSION__ for stamped builds:

declare const __VERSION__: string;
const version = __VERSION__;

This matches what @spentacular flagged in the inline comment.

@rafbcampos
Copy link
Contributor Author

@claude can you do another review and check if we addressed all the issues pointed in the last one?

@claude
Copy link

claude bot commented Mar 25, 2026

Claude finished @rafbcampos's task in 1m 2s —— View job


All four bugs from the previous reviews have been addressed. Three of the five design notes are still open but are non-blocking.

Bug fixes — status

# Issue Status
1 LRUCache.has() promotes to MRU ✅ Fixed — now delegates to this.cache.has(key) directly
2 "..." appended even when not truncated ✅ Fixed — correctly checks full.length > config.overviewMaxLength before appending
3 parseInt of env vars can return NaN ✅ Fixed — all three values now have Number.isNaN() fallbacks
4 version should use __VERSION__ ✅ Addressed — declare const __VERSION__: string added with a runtime typeof guard as fallback

Design notes — still open (non-blocking)

Double iteration in get-package.tslines 45–58

Two separate loops iterate over knowledgeMap — first for the main package, then for dependencies. Functionally correct, but a single pass with a conditional is cleaner and more readable.

Redundant second condition in path traversal checkdetect-packages.ts:37–38

resolve(baseDir, relativePath) !== resolvedPath

relativePath = relative(baseDir, resolvedPath), so resolve(baseDir, relativePath) always equals resolvedPath — this branch is dead code. The startsWith("..") check (line 37) is the meaningful guard.

Also, the startsWith(baseDir) check at line 44 has a potential false positive on paths like /foo/bar-extra when baseDir=/foo/bar. Should use startsWith(baseDir + path.sep) || resolvedPath === baseDir.

server.server accesses internal SDK propertyindex.ts:135

Still using server.server.setRequestHandler(...). A brief comment explaining why the high-level server.tool() API isn't used here (likely for dynamic enum in scope/category schemas) would help future maintainers.


The PR is in good shape — all flagged bugs are resolved. The remaining items are minor. LGTM.

@rafbcampos
Copy link
Contributor Author

/canary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants