Skip to content
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

feat: UI Embeddable and Micro App #407

Closed
wants to merge 24 commits into from
Closed

feat: UI Embeddable and Micro App #407

wants to merge 24 commits into from

Conversation

nadeesha
Copy link
Contributor

@nadeesha nadeesha commented Dec 27, 2024

PR Type

Enhancement


Description

  • Implemented a new UI Embeddable adapter for chat interface integration
  • Created comprehensive API contract definitions using ts-rest and Zod for type safety
  • Developed MicroApp component with full chat functionality and custom authentication
  • Added Message component for displaying various types of chat messages
  • Implemented responsive styling with CSS variables for consistent theming
  • Updated dependencies to use @inferable/react instead of previous UI packages
  • Added support for clusterId and custom authentication token configuration

Changes walkthrough 📝

Relevant files
Enhancement
contract.ts
API Contract Definitions for UI Embeddable Adapter             

adapters/ui-embeddable/components/contract.ts

  • Implemented comprehensive API contract definitions using ts-rest and
    Zod
  • Defined schemas for machine headers, messages, functions, and various
    API endpoints
  • Added extensive type validation for API requests and responses
  • +1532/-0
    page.tsx
    Implement Main Page with MicroApp Integration                       

    adapters/ui-embeddable/app/page.tsx

  • Replaced default Next.js template with MicroApp component
  • Added clusterId and customAuthToken configuration
  • Simplified page layout to focus on chat interface
  • +11/-95 
    micro-app.tsx
    MicroApp Component Implementation with Chat Interface       

    adapters/ui-embeddable/components/micro-app.tsx

  • Created MicroApp component with chat interface functionality
  • Implemented message handling and UI state management
  • Added support for custom authentication and clustering
  • +121/-0 
    message.tsx
    Message Component for Chat Interface Display                         

    adapters/ui-embeddable/components/message.tsx

  • Implemented Message component for chat messages display
  • Added support for different message types (human, agent, invocation)
  • Included expandable result view for invocation results
  • +73/-0   
    client.tsx
    API Client Implementation with Custom Auth                             

    adapters/ui-embeddable/components/client.tsx

  • Created API client initialization with custom auth token
  • Added baseUrl configuration for server/client environments
  • Implemented useClient hook for React integration
  • +30/-0   
    micro-app.css
    Styling Implementation for MicroApp Component                       

    adapters/ui-embeddable/components/micro-app.css

  • Defined root variables for consistent theming
  • Implemented styles for chat interface components
  • Added responsive design elements and animations
  • +135/-0 
    message.css
    Message Component Styling Implementation                                 

    adapters/ui-embeddable/components/message.css

  • Implemented styles for different message types
  • Added styling for message bubbles and containers
  • Defined typography and spacing for chat messages
  • +99/-0   
    Additional files (token-limit)
    package-lock.json
    ...                                                                                                           

    adapters/ui-embeddable/package-lock.json

    ...

    +114/-1099

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @nadeesha nadeesha marked this pull request as ready for review December 27, 2024 21:25
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Authentication token exposure:
    The customAuthToken is passed directly as a prop and used in API calls. Consider implementing proper token management and not exposing authentication tokens directly in the UI components.

    ⚡ Recommended focus areas for review

    Error Handling

    The error handling in useRun hook only logs to console. Consider displaying errors to users and implementing proper error recovery mechanisms.

    onError: error => {
      console.error(error);
    },
    authType: "custom",
    Environment Variables

    The clusterId environment variable is required but there's no fallback or proper error message for users if not configured correctly.

    const clusterId = process.env.NEXT_PUBLIC_TEST_INFERABLE_CLUSTER_ID;
    
    if (!clusterId) {
      throw new Error("NEXT_PUBLIC_TEST_INFERABLE_CLUSTER_ID is not set");
    }

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 27, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add numerical bounds validation to prevent invalid parameter values

    Add validation for the temperature range in createStructuredOutput to ensure it
    stays within reasonable model limits (typically 0.0-1.0).

    adapters/ui-embeddable/components/contract.ts [1500-1504]

     temperature: z
       .number()
    +  .min(0)
    +  .max(1)
       .optional()
    -  .describe("The temperature to use for the model")
    +  .describe("The temperature to use for the model (0.0-1.0)")
       .default(0.5),
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding min/max validation for the temperature parameter is crucial to prevent invalid values that could affect model behavior. This is a critical validation for API stability.

    9
    Update React dependency to a valid version number to ensure compatibility and stability

    The React version specified (^19.0.0) is not a valid version. React's latest major
    version is 18.x. Update to a valid React version to avoid compatibility issues.

    adapters/ui-embeddable/package.json [14-15]

    -"react": "^19.0.0",
    -"react-dom": "^19.0.0"
    +"react": "^18.2.0",
    +"react-dom": "^18.2.0"
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: React version ^19.0.0 is invalid as React 19 has not been released yet. Using an invalid version could cause dependency resolution issues and potential build failures.

    9
    Replace non-null assertion with proper type checking to prevent runtime errors

    Add error handling for the case when clusterId is undefined. The current code uses
    the non-null assertion operator (!), which could lead to runtime errors. Instead,
    implement proper type checking and error handling.

    adapters/ui-embeddable/app/page.tsx [13]

    -<MicroApp initialMessage="Is the server running?" clusterId={clusterId!} customAuthToken="test" />
    +{clusterId && <MicroApp initialMessage="Is the server running?" clusterId={clusterId} customAuthToken="test" />}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using non-null assertion (!) can lead to runtime errors if clusterId is undefined. The suggested type-safe approach prevents potential crashes and improves code reliability.

    8
    Security
    Move authentication token handling to server-side to prevent exposure of sensitive credentials

    The customAuthToken is hardcoded as "test" and exposed in the client-side code,
    which is a security risk. Move authentication token handling to a secure server-side
    implementation.

    adapters/ui-embeddable/components/micro-app.tsx [21-28]

    -export default function App({ buttonText, initialMessage, clusterId, customAuthToken }: AppProps) {
    +export default function App({ buttonText, initialMessage, clusterId }: Omit<AppProps, 'customAuthToken'>) {
       const { createMessage, messages, run } = useRun({
         clusterId,
    -    customAuthToken,
    +    authType: "custom",
         onError: error => {
           console.error(error);
         },
    -    authType: "custom",
    +    getAuthToken: async () => {
    +      const response = await fetch('/api/auth/token');
    +      const { token } = await response.json();
    +      return token;
    +    }
       });
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Critical security improvement that prevents exposing authentication tokens in client-side code by moving token handling to a secure server-side implementation.

    9
    General
    Use a more explicit schema definition for better type safety and validation

    Add a more specific type for the anyObject schema instead of using an empty object
    with passthrough. This could allow unexpected properties and types to pass
    validation.

    adapters/ui-embeddable/components/contract.ts [23]

    -const anyObject = z.object({}).passthrough();
    +const anyObject = z.record(z.unknown());
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using z.record(z.unknown()) provides better type safety and explicit validation compared to an empty object with passthrough, which could allow unexpected data to pass validation.

    8
    Remove redundant optional chaining that could lead to confusion about validation behavior

    Remove the duplicate .optional() chaining in the x-forwarded-for and
    x-sentinel-no-mask header definitions. Having multiple .optional() calls is
    redundant and could cause confusion.

    adapters/ui-embeddable/components/contract.ts [10-11]

    -"x-forwarded-for": z.string().optional().optional(),
    -"x-sentinel-no-mask": z.string().optional().optional(),
    +"x-forwarded-for": z.string().optional(),
    +"x-sentinel-no-mask": z.string().optional(),
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Duplicate .optional() calls are redundant and could cause confusion about the validation behavior. Removing them makes the code clearer and maintains the same functionality.

    7
    Improve error handling by displaying user-friendly error messages in the UI

    Add proper error handling and user feedback for failed message creation attempts
    instead of just logging to console.

    adapters/ui-embeddable/components/micro-app.tsx [24-26]

    +const [error, setError] = useState<string | null>(null);
    +// ...
     onError: error => {
       console.error(error);
    +  setError('Failed to send message. Please try again.');
    +  setIsThinking(false);
     },
    +// ... and in the JSX:
    +{error && <div className="error-message">{error}</div>}
    Suggestion importance[1-10]: 7

    Why: Enhances user experience by providing visible feedback for errors instead of just logging to console, making the application more user-friendly and maintainable.

    7

    },
    });

    export const useClient = (customAuthToken: string) => {
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Is this used?

    Are there endpoints we need access to that aren't available through useRun?

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 28, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit 0e12fd3)

    Action: test-react

    Failed stage: Run tests [❌]

    Failed test name: should create a message

    Failure summary:

    The action failed due to a test failure in the useRun hook test suite. Specifically, the test
    "should create a message" failed because a required runId parameter was missing, triggering the
    error "Run ID is required" in the createMessage function.

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    170:  �[90m 137 |�[39m       params�[33m:�[39m {
    171:  �[90m 138 |�[39m         clusterId�[33m:�[39m �[32m"test-cluster"�[39m�[33m,�[39m
    172:  �[90m 139 |�[39m         runId�[33m,�[39m�[0m
    173:  at Object.<anonymous> (src/hooks/useRun.test.ts:136:40)
    174:  ● useRun › should create a message
    175:  Run ID is required
    176:  �[0m �[90m 171 |�[39m     �[36masync�[39m (input�[33m:�[39m �[33mCreateMessageInput�[39m) �[33m=>�[39m {
    177:  �[90m 172 |�[39m       �[36mif�[39m (�[33m!�[39mrunId�[33m.�[39mcurrent) {
    178:  �[31m�[1m>�[22m�[39m�[90m 173 |�[39m         �[36mthrow�[39m �[36mnew�[39m �[33mError�[39m(�[32m"Run ID is required"�[39m)�[33m;�[39m
    ...
    
    181:  �[90m 175 |�[39m
    182:  �[90m 176 |�[39m       �[36mconst�[39m response �[33m=�[39m �[36mawait�[39m client�[33m.�[39mcreateMessage({�[0m
    183:  at Object.createMessage (src/hooks/useRun.ts:173:15)
    184:  at src/hooks/useRun.test.ts:177:28
    185:  at node_modules/@testing-library/react/dist/act-compat.js:48:24
    186:  at act (node_modules/react/cjs/react.development.js:2512:16)
    187:  at node_modules/@testing-library/react/dist/act-compat.js:47:25
    188:  at Object.<anonymous> (src/hooks/useRun.test.ts:176:14)
    189:  Test Suites: 1 failed, 1 total
    190:  Tests:       3 failed, 2 passed, 5 total
    191:  Snapshots:   0 total
    192:  Time:        5.345 s
    193:  Ran all test suites.
    194:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @nadeesha nadeesha closed this Dec 30, 2024
    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