Minor fixes 0821 #1385
Minor fixes 0821 #1385
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds conditional HyprMCP client initialization and tool ingestion for pro.hyprnote.com, a utility to wrap MCP tools as Vercel dynamic tools, a Pencil action to focus the transcript editor, re-exports Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Chat UI
participant Hook as useChatLogic
participant Lic as License API
participant MCP as HyprMCP Server
participant Stream as streamText
UI->>Hook: start chat (shouldUseTools?)
alt apiBase includes pro.hyprnote.com AND license valid
Hook->>Lic: getLicenseKey()
Lic-->>Hook: licenseKey
Hook->>MCP: connect via StreamableHTTPClientTransport (Authorization: licenseKey)
Note over Hook,MCP: listTools() and buildVercelToolsFromMcp()
MCP-->>Hook: tools[]
Hook->>Hook: create hyprMcpTools
else
Note over Hook: Skip HyprMCP init
end
Hook->>Stream: streamText({ tools: newMcpTools + hyprMcpTools })
Stream-->>Hook: stream events
Hook-->>UI: update chat stream
UI-->>Hook: finish
Hook->>MCP: close() (if initialized)
Note over Hook,MCP: Cleanup HyprMCP client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
apps/desktop/package.json (1)
43-45: Dependency addition LGTM; align deep-import usage across codeAdding
@modelcontextprotocol/sdk@^1.17.3is appropriate for the new client/HTTP transport usage.You’re mixing deep imports with and without “.js” extensions in different files. Standardize on one form (prefer extensionless entrypoints) to avoid duplicate module instances in bundlers. See inline suggestions in the TS files’ comments.
apps/desktop/src/components/right-panel/views/transcript-view.tsx (1)
184-193: Add accessible label and unify icon size with neighborsThe icon-only button should expose an accessible name; also consider matching the 14px size used by other header icons for visual consistency.
Apply:
- <Button + <Button className="w-8 h-8" variant="ghost" size="icon" onClick={handleFocusEditor} + aria-label="Focus editor" + title="Focus editor" > - <PencilIcon size={12} className="text-neutral-600" /> + <PencilIcon size={14} className="text-neutral-600" /> </Button>apps/desktop/src/components/right-panel/utils/mcp-http-wrapper.ts (4)
2-2: Use a type-only import for Client to avoid an unnecessary runtime dependency
Clientis only used for typing here; importing it as a value can pull runtime code into the bundle unnecessarily (and may trigger linter rules about unused value imports).-import { Client } from "@modelcontextprotocol/sdk/client"; +import type { Client } from "@modelcontextprotocol/sdk/client";
5-6: Add an explicit return type for clarity and IDE helpBeing explicit here makes call sites clearer and improves autocomplete.
-export async function buildVercelToolsFromMcp(client: Client) { +export async function buildVercelToolsFromMcp( + client: Client +): Promise<Record<string, ReturnType<typeof dynamicTool>>> {
11-13: Avoidanycast on JSON SchemaPrefer
unknown(or the concrete JSON Schema type if available) to reduce type unsafety while keeping flexibility.- const schema = mcpTool.inputSchema - ? jsonSchema(mcpTool.inputSchema as any) + const schema = mcpTool.inputSchema + ? jsonSchema(mcpTool.inputSchema as unknown) : z.any();
19-26: Consider normalizing tool return shapeReturning raw
result.content(often an array of MCP content parts) is fine, but downstream tooling sometimes expects a JSON object. If you see odd rendering/serialization, wrap in an object (e.g.,{ content: result.content }). No change required now; just a heads-up.apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (6)
1-2: Standardize MCP SDK deep imports (avoid “.js” extensions)You’re importing with extensions here, while other files use extensionless paths. Mixing styles can produce duplicate module instances in certain bundlers. Prefer extensionless entrypoints consistently.
-import { Client } from "@modelcontextprotocol/sdk/client/index.js"; -import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; +import { Client } from "@modelcontextprotocol/sdk/client"; +import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp";
179-183: TightenhyprMcpToolstyping for safety and editor hintsThis narrows
anyto the actual dynamic tool type used elsewhere.- let hyprMcpTools: Record<string, any> = {}; + let hyprMcpTools: Record<string, ReturnType<typeof dynamicTool>> = {};
194-221: HyprMCP client init looks solid; minor nits on identifiersThe transport, header injection, and connection flow look correct. Optional: pick a more specific client name (e.g.,
"hyprmcp-desktop") to make server-side diagnostics clearer.
358-360: Remove “what” comment; the code is self-explanatoryPer guidelines, prefer comments explaining “why.” This one narrates “what.”
- // close hyprmcp client hyprMcpClient?.close();
184-189: Tooling activation predicate: extract to a helper for reuse and testsThe model-id checks and
apiBasegating are getting long. Consider a tiny helper likesupportsTools(modelId: string, apiBase?: string), making it easier to test and reuse.
264-271: Detect and warn on duplicate tool keys before mergingThe spread merge at line 346 silently overrides any keys present in both
newMcpToolsandhyprMcpTools, which can hide unintended collisions.• File: apps/desktop/src/components/right-panel/hooks/useChatLogic.ts
Location: around thetools: { ...newMcpTools, search_sessions_multi_keywords: searchTool, ...hyprMcpTools }statement (line 346).
• Risk: If a tool key exists in both sources, the HyprMCP version will overwrite the MCP server version without any notice.
• Recommendation: Before merging, compute the intersection of the two key sets and emit a warning (or throw) when duplicates are found. For example:const hyprKeys = Object.keys(hyprMcpTools); const mcpKeys = Object.keys(newMcpTools); const dupKeys = mcpKeys.filter(key => hyprKeys.includes(key)); if (dupKeys.length > 0) { console.warn( `Duplicate tool keys detected (newMcpTools will be overwritten by hyprMcpTools): ${dupKeys.join( ', ', )}`, ); } const mergedTools = { ...newMcpTools, search_sessions_multi_keywords: searchTool, ...hyprMcpTools, };This makes any name collision explicit and aids future debugging.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/desktop/package.json(1 hunks)apps/desktop/src/components/right-panel/hooks/useChatLogic.ts(8 hunks)apps/desktop/src/components/right-panel/utils/mcp-http-wrapper.ts(1 hunks)apps/desktop/src/components/right-panel/views/transcript-view.tsx(3 hunks)packages/utils/src/ai.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
packages/utils/src/ai.tsapps/desktop/src/components/right-panel/views/transcript-view.tsxapps/desktop/src/components/right-panel/utils/mcp-http-wrapper.tsapps/desktop/src/components/right-panel/hooks/useChatLogic.ts
🔇 Additional comments (2)
packages/utils/src/ai.ts (1)
8-19: jsonSchema re-export verified
- Workspace packages (
@hypr/adminand@hypr/utils) depend onai@^5.0.21, which includes a root-level re-export ofjsonSchemafrom@ai-sdk/provider-utilsin itsdist/index.js- No direct imports of
jsonSchemafrom"ai"were detected anywhere in the codebaseThe change to re-export
jsonSchemafrom"ai"inpackages/utils/src/ai.tsis fully supported by the versions in use and introduces no runtime impact. Resolving.apps/desktop/src/components/right-panel/views/transcript-view.tsx (1)
134-139: Editor focus handler is simple and correctThe null checks against
editorRef.current?.editorare sufficient, and gating viashowActionsprevents focusing during live sessions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/components/right-panel/components/chat/empty-chat-state.tsx (1)
35-35: Confirm new settings tab slug and consider centralizing it to avoid driftRoute changed to “/app/settings?tab=ai-llm”. Please verify that the router recognizes
ai-llmand that any other deep-links (or default tab logic) have been updated fromaitoai-llmto keep navigation consistent. As a minor hardening against future renames, consider extracting this path into a shared constant.Apply this minimal diff to reference a constant:
- windowsCommands.windowNavigate({ type: "settings" }, "/app/settings?tab=ai-llm"); + windowsCommands.windowNavigate({ type: "settings" }, SETTINGS_AI_LLM_TAB);And add the constant near the imports (or in a shared routes/constants module if you have one):
const SETTINGS_AI_LLM_TAB = "/app/settings?tab=ai-llm";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src/components/right-panel/components/chat/empty-chat-state.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/right-panel/components/chat/empty-chat-state.tsx
⏰ 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: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
Summary by cubic
Enables Hypr MCP tool execution in chat for licensed users and adds a quick Edit button in the transcript bar to focus the editor. Also exposes jsonSchema to support MCP tool schemas.
New Features
Dependencies