refactor(ui): Simplify UI package code for clarity and maintainability (Vibe Kanban)

[gabe4coding]

Summary

This PR simplifies and refines the @mcp-apps-kit/ui package code for improved clarity, consistency, and maintainability while preserving all functionality.

Changes Made

src/index.ts

• Removed duplicate "CLIENT FACTORY" section headers and consolidated imports that were declared twice

src/client.ts

• Simplified optional file method handling by using .bind(adapter) instead of wrapper functions with redundant null checks
• Reduced 14 lines to 4 lines while maintaining the same behavior

src/detection.ts

• Inlined the isChatGPTSandbox() helper function into detectProtocol() since it was only used once
• Simplified the logic into a single boolean expression, reducing 36 lines to 11 lines

src/adapters/mcp.ts

• Replaced nested ternary operator in mapHostContext() with clearer if-statement for theme detection
• Simplified console log mapping to be consistent with other adapters

src/adapters/mock.ts & src/adapters/openai.ts

• Unified console log mapping pattern across all adapters for consistency
• Reduced boilerplate by using inline object lookup instead of separate variable declaration

Impact

• Lines removed: 76 → 34 (net reduction of 42 lines)
• All 174 tests pass ✅
• TypeScript compilation ✅
• ESLint ✅
• Build succeeds ✅

---

This PR was written using Vibe Kanban

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Refactor
  • Internal code improvements and simplifications to adapter implementations and client utilities. Theme derivation logic, logging mechanisms, and file operation handlers have been streamlined for improved maintainability. Sandbox detection logic has been optimized through direct inline checks.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Walkthrough

This PR refactors adapter implementations and detection logic across multiple files. Changes simplify logging implementations in adapters, replace wrapper methods with direct bindings in the client, inline sandbox detection logic, and reorganize export declarations. No functional behavior modifications.

Changes

Cohort / File(s)	Summary
Adapter logging refactoring packages/ui/src/adapters/mcp.ts, packages/ui/src/adapters/mock.ts, packages/ui/src/adapters/openai.ts	Simplified logging implementations across adapters: replaced separate logMapping objects with inline object literals for console method selection, with fallback to console.log for unrecognized levels. Theme derivation in mcp.ts refactored from nested ternary to explicit conditional variable. ESLint console comments added to suppress linting warnings.
Client method binding refactoring packages/ui/src/client.ts	Replaced wrapper function implementations for uploadFile and getFileDownloadUrl with direct method binding using .bind(adapter), removing intermediate existence checks and throw paths.
Detection logic refactoring packages/ui/src/detection.ts	Removed isChatGPTSandbox() helper function and inlined sandbox detection logic directly within detectProtocol() using combined URL and referrer checks.
Export reorganization packages/ui/src/index.ts	Relocated createAppsClient export from internal section to after import block. No signature or public API changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• Fix linting errors and warnings #4 — Contains overlapping logging refactoring changes to mcp.ts, mock.ts, and openai.ts with identical inline mapping and eslint patterns.
• Add support for MCP Apps capabilities and versioning #56 — Modifies adapter logging fallbacks and host theme derivation logic in the same adapter files (mcp.ts, mock.ts, openai.ts).

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main objective of the PR: simplifying the UI package code for clarity and maintainability.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, detailing specific simplifications made across multiple files and including verification that all tests pass.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ba40c19 and e326ff0.

📒 Files selected for processing (6)

• packages/ui/src/adapters/mcp.ts
• packages/ui/src/adapters/mock.ts
• packages/ui/src/adapters/openai.ts
• packages/ui/src/client.ts
• packages/ui/src/detection.ts
• packages/ui/src/index.ts

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Use strict TypeScript with no any types - use unknown and narrow instead

Files:

• packages/ui/src/adapters/mcp.ts
• packages/ui/src/adapters/mock.ts
• packages/ui/src/detection.ts
• packages/ui/src/index.ts
• packages/ui/src/adapters/openai.ts
• packages/ui/src/client.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Remove unused variables or prefix with underscore (_)

Files:

• packages/ui/src/adapters/mcp.ts
• packages/ui/src/adapters/mock.ts
• packages/ui/src/detection.ts
• packages/ui/src/index.ts
• packages/ui/src/adapters/openai.ts
• packages/ui/src/client.ts

**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Use export type for type-only exports

Files:

• packages/ui/src/adapters/mcp.ts
• packages/ui/src/adapters/mock.ts
• packages/ui/src/detection.ts
• packages/ui/src/index.ts
• packages/ui/src/adapters/openai.ts
• packages/ui/src/client.ts

**/index.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Export public API only in index.ts files

Files:

• packages/ui/src/index.ts

🧠 Learnings (3)

📚 Learning: 2025-12-29T20:58:59.376Z

Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Colocate UI definitions near tool definitions using `defineUI` with `html` property pointing to compiled UI assets

Applied to files:

• packages/ui/src/index.ts

📚 Learning: 2025-12-29T20:58:59.376Z

Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/index.ts : Export public API only in `index.ts` files

Applied to files:

• packages/ui/src/index.ts

📚 Learning: 2025-12-29T20:58:59.376Z

Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components

Applied to files:

• packages/ui/src/index.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: test-and-lint
• GitHub Check: claude-review

🔇 Additional comments (7)

packages/ui/src/adapters/mock.ts (1)

180-188: LGTM — Clean simplification of log level mapping.

The inline object lookup with nullish coalescing fallback is concise and maintains identical behavior. ESLint disable/enable comments are correctly scoped.

packages/ui/src/adapters/openai.ts (1)

608-616: LGTM — Consistent log mapping format across adapters.

The reformatted inline mapping aligns with the simplification pattern applied in mock.ts and mcp.ts.

packages/ui/src/adapters/mcp.ts (2)

242-245: LGTM — Clearer theme derivation logic.

Replacing the nested ternary with explicit initialization and conditional override improves readability while maintaining equivalent behavior.

---

465-471: LGTM — Consistent log fallback pattern.

The inline mapping matches the simplified pattern used across all adapters.

packages/ui/src/detection.ts (1)

34-46: LGTM — Inlined sandbox detection logic.

The inline approach is appropriate since this logic is used only once. The detection patterns remain unchanged.

One minor note: the includes() checks are case-sensitive. This is likely intentional given typical URL conventions, but could miss edge cases like ChatGPT in the URL.

packages/ui/src/index.ts (1)

119-120: LGTM — Export relocation maintains public API surface.

The re-export of createAppsClient is correctly placed after its import, following the pattern for exposing public API through index.ts. Based on learnings, this aligns with the guideline to export public API only in index.ts files.

packages/ui/src/client.ts (1)

146-151: LGTM — Simplified optional method delegation.

Using .bind(adapter) directly is cleaner than wrapper functions while correctly preserving this context. The conditional spread still ensures these properties are only added when the adapter implements them.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review: Simplify UI Package

Thank you for this refactoring PR! The changes successfully simplify the code and improve readability. Here's my detailed review:

---

✅ Positive Changes

1. Improved Code Clarity (detection.ts)

• Inlining isChatGPTSandbox(): Good decision to inline this single-use function. The boolean expression is clear and eliminates unnecessary function call overhead.
• Self-documenting: The combined conditions make the ChatGPT detection logic immediately visible.

2. Cleaner Console Logging Pattern

All three adapters (mcp.ts, mock.ts, openai.ts) now use a consistent, concise pattern:

const logFn = { debug: console.debug, info: console.info, ... }[level] ?? console.log;

This is more readable than the previous Record<> approach.

3. Simplified Method Binding (client.ts)

Using .bind(adapter) for optional methods is cleaner:

uploadFile: adapter.uploadFile.bind(adapter)

This removes redundant null checks and error throwing that could never be reached.

4. Fixed Export Organization (index.ts)

Removing the duplicate "CLIENT FACTORY" comment header and consolidating the export is good housekeeping.

---

⚠️ Areas of Concern

1. Potential this Binding Issue (client.ts:147, 150)

Severity: Medium

The change from explicit null checks to .bind(adapter) assumes the adapter methods don't rely on internal this context. While .bind() should handle this correctly, if the adapter methods access other instance properties, this could introduce subtle bugs.

Recommendation:

• Verify that uploadFile and getFileDownloadUrl in all adapters (McpAdapter, OpenAIAdapter, MockAdapter) don't have dependencies on this context beyond what .bind() preserves
• Consider adding integration tests that specifically test these file operations through the client

Example Test:

it('should correctly bind uploadFile method', async () => {
  const client = createAppsClient({ protocol: 'openai' });
  const mockFile = new File(['test'], 'test.txt');
  // Verify the bound method works correctly
  await expect(client.uploadFile(mockFile)).resolves.toBeDefined();
});

2. Theme Handling Logic Change (mcp.ts:242-245)

Severity: Low

The refactored theme logic is functionally equivalent but slightly less explicit:

Before:

const theme = ctx.theme === 'dark' ? 'dark' : ctx.theme === 'light' ? 'light' : base.theme;

After:

let theme: 'light' | 'dark' = base.theme;
if (ctx.theme === 'dark' || ctx.theme === 'light') {
  theme = ctx.theme;
}

While the new version is more readable, it uses a type assertion internally. TypeScript knows ctx.theme is 'dark' | 'light' inside the if block, which is good. No action needed, but worth noting the subtle difference.

---

🧪 Test Coverage

Good:

• Detection logic is covered by detection.test.ts
• Adapter methods are covered by individual adapter tests
• The inlined ChatGPT detection is implicitly tested

Gap:

• No explicit tests for the .bind() changes in client.ts
• File operation tests in openai-adapter.test.ts only verify method existence, not actual binding behavior through the client

Recommendation:
Add a test in client.test.ts that verifies file operations work correctly when called through the client interface.

---

🔒 Security Considerations

No security concerns identified. The changes are purely refactoring with no impact on:

• Input validation
• Authentication
• Data handling
• External API calls

---

⚡ Performance

Minor improvement:

• Inlining isChatGPTSandbox() eliminates one function call during protocol detection
• Using object literals instead of Record<> for log mapping has negligible performance impact but improves readability

---

📊 Code Quality Metrics

• Lines of Code: -76 additions, +34 deletions = -42 net lines ✅
• Complexity: Reduced (fewer function calls, simpler patterns)
• Maintainability: Improved (more consistent patterns across adapters)
• Adherence to Guidelines: Follows CLAUDE.md conventions ✅

---

📝 Recommendations

1. High Priority: Add integration test for file operation binding in client.ts
2. Medium Priority: Verify adapter methods don't have hidden this dependencies
3. Low Priority: Consider adding a comment explaining why .bind() is used for optional methods

---

✨ Summary

This is a solid refactoring PR that improves code quality without changing functionality. The main risk is the .bind() change for optional methods, which should be verified with integration tests. Otherwise, the changes are well-structured and improve maintainability.

Recommendation: Approve with minor test additions

---

Review performed by Claude Code following mcp-apps-kit coding standards