Skip to content

chore: support lucien deeplink in registry page #191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 30, 2025

Conversation

JoJoJoJoJoJoJo
Copy link
Contributor

@JoJoJoJoJoJoJo JoJoJoJoJoJoJo commented Jun 28, 2025

User description

  • support lucien deep link
  • support pinned mcp servers and sort servers by: 1. pinned 2. official 3. others

PR Type

Enhancement


Description

  • Add Lucien deeplink support for MCP server installation

  • Implement pinned server sorting with priority order

  • Add install buttons with deeplink functionality

  • Create server configuration for Lucien integration


Changes diagram

flowchart LR
  A["Registry Page"] --> B["Server Sorting"]
  A --> C["Install Button"]
  B --> D["Pinned Servers"]
  B --> E["Official Servers"]
  B --> F["Other Servers"]
  C --> G["Lucien Deeplink"]
  H["Lucien Config"] --> D
Loading

Changes walkthrough 📝

Relevant files
Enhancement
index.html
Add Lucien deeplink and server sorting                                     

pages/registry/index.html

  • Add CSS styling for install buttons with positioning
  • Implement server sorting logic with pinned/official priority
  • Add install button with deeplink functionality for Lucien
  • Include Lucien server configuration and URL parameter handling
  • +109/-0 
    Configuration changes
    lucien-servers.js
    Create Lucien server configuration                                             

    pages/_includes/lucien-servers.js

    • Define pinned servers list for Lucien integration
    +5/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @JoJoJoJoJoJoJo JoJoJoJoJoJoJo marked this pull request as ready for review June 28, 2025 16:00
    @Copilot Copilot AI review requested due to automatic review settings June 28, 2025 16:00
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Deeplink injection:
    The code constructs deeplinks using server data without proper validation or sanitization. If malicious server data is present, it could potentially construct harmful URLs. The server name, command, args, and env parameters are directly passed to the deeplink construction without validation, which could be exploited to execute unintended commands or access unauthorized resources.

    ⚡ Recommended focus areas for review

    Security Risk

    The deeplink functionality directly uses user input to construct URLs without validation. The server name and installation parameters are passed directly from the server data to construct the deeplink, which could potentially be exploited if malicious server data is injected.

    installButton.addEventListener('click', (event) => {
        // Prevent the modal from opening
        event.stopPropagation();
    
        // Build search params with server install info
        const params = new URLSearchParams();
        params.append('name', server.name);
        params.append('action', 'install');
    
        const installInfo = Object.values(server.installations)[0];
        // No remote for now, only stdio
        params.append('command', installInfo.command);
        params.append('args', installInfo.args.join(' '));
        if (installInfo.env) {
            params.append('env', JSON.stringify(installInfo.env));
        }
    
        console.log(params)
        // Create and trigger the deeplink
        const deeplink = `${sourceProtocol}://mcpm?${params.toString()}`;
        window.location.href = deeplink;
    Error Handling

    The install button click handler assumes the first installation method exists and has required properties. If server.installations is empty or the first installation lacks command/args properties, this will cause runtime errors.

    const installInfo = Object.values(server.installations)[0];
    // No remote for now, only stdio
    params.append('command', installInfo.command);
    params.append('args', installInfo.args.join(' '));
    if (installInfo.env) {
        params.append('env', JSON.stringify(installInfo.env));
    }
    Code Quality

    Debug console.log statement left in production code should be removed. Also, the comment about "No remote for now, only stdio" suggests incomplete implementation that may need addressing.

    console.log(params)
    // Create and trigger the deeplink

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 28, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null safety checks

    Add null checks before accessing properties to prevent runtime errors when
    server.installations is empty or undefined. This prevents crashes when servers
    have missing installation data.

    pages/registry/index.html [2103-2109]

    -const installInfo = Object.values(server.installations)[0];
    +const installations = server.installations;
    +if (!installations || Object.keys(installations).length === 0) {
    +    return; // Skip if no installations available
    +}
    +const installInfo = Object.values(installations)[0];
     // No remote for now, only stdio
     params.append('command', installInfo.command);
     params.append('args', installInfo.args.join(' '));
     if (installInfo.env) {
         params.append('env', JSON.stringify(installInfo.env));
     }
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that server.installations could be missing or empty, which would cause a runtime error when Object.values(server.installations)[0] is evaluated. Adding a check to handle this case improves the code's robustness.

    Medium
    Validate args before joining

    Check if installInfo.args exists and is an array before calling join() to
    prevent TypeError when args property is missing or not an array.

    pages/registry/index.html [2106]

    -params.append('args', installInfo.args.join(' '));
    +params.append('args', (installInfo.args && Array.isArray(installInfo.args)) ? installInfo.args.join(' ') : '');
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: This is a valid suggestion that prevents a potential TypeError if installInfo.args is not an array or is undefined. The proposed change ensures the code handles this edge case gracefully, improving its reliability.

    Medium
    General
    Remove debug console statement
    Suggestion Impact:The console.log(params) statement was removed from line 56 in the commit, exactly as suggested

    code diff:

    -                    console.log(params)

    Remove debug console.log statement from production code to avoid cluttering
    browser console and potentially exposing sensitive information.

    pages/registry/index.html [2111]

    -console.log(params)
     
    +

    [Suggestion processed]

    Suggestion importance[1-10]: 3

    __

    Why: The suggestion correctly points out a leftover console.log statement. While removing it is good practice for code hygiene before merging to production, it has a low impact on functionality or correctness.

    Low
    • Update

    Copilot

    This comment was marked as outdated.

    Copy link
    Contributor

    @Copilot Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    Adds support for Lucien deep links on the registry page and implements a pinned/official/other server sorting order.

    • Introduces an “Install” button with Lucien deeplink functionality
    • Implements sorting logic to prioritize pinned, then official, then other servers
    • Provides a small Lucien server configuration file for specifying pinned servers

    Reviewed Changes

    Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

    File Description
    pages/registry/index.html Added CSS for .install-button, included Lucien config, sorting logic, and deeplink handlers
    pages/_includes/lucien-servers.js Defined lucienSpecifiedServers array for pinned-server support

    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo enabled auto-merge (squash) June 30, 2025 06:54
    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo disabled auto-merge June 30, 2025 06:55
    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo enabled auto-merge (squash) June 30, 2025 06:56
    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo disabled auto-merge June 30, 2025 06:56
    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo merged commit 264fc25 into main Jun 30, 2025
    6 checks passed
    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo deleted the jonathan/chore-support-lucien-install-deeplink branch June 30, 2025 06:57
    @mcpm-semantic-release
    Copy link

    🎉 This PR is included in version 1.14.1 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

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

    Successfully merging this pull request may close these issues.

    2 participants