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: Allow useRun to specify a resultSchema #408

Merged
merged 4 commits into from
Dec 28, 2024

Conversation

nadeesha
Copy link
Contributor

@nadeesha nadeesha commented Dec 28, 2024

PR Type

Enhancement


Description

  • Added Zod schema validation support to the useRun hook, allowing type-safe validation of run results
  • Implemented generic type parameter T extends z.ZodObject<any> for strong typing of result schemas
  • Enhanced the hook's return type to include validated result data
  • Updated demo page to showcase schema validation usage with a sample schema
  • Improved type definitions and added proper TypeScript generics support
  • Simplified component initialization by removing redundant start function

Changes walkthrough 📝

Relevant files
Enhancement
TestPage.tsx
Add Zod schema validation to demo page                                     

sdk-react/demo/TestPage.tsx

  • Added Zod schema validation integration with resultSchema option
  • Updated useRun hook configuration with custom auth token
  • Removed start function usage and simplified component initialization
  • Code formatting and style improvements
  • +53/-46 
    useRun.ts
    Implement Zod schema validation in useRun hook                     

    sdk-react/src/hooks/useRun.ts

  • Added Zod schema validation support with generic type parameter
  • Integrated result schema validation in run creation
  • Added new result field in UseRunReturn with typed validation
  • Enhanced type definitions for better type safety
  • +10/-3   

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Safety

    The type casting of resultSchema to 'any' in createRun call could bypass type checking. Consider using proper type constraints or validation.

    resultSchema: options.resultSchema as any,
    Error Handling

    The safeParse() result is not properly handled - it could fail silently if the schema validation fails. Consider adding error handling for failed validations.

    result: run?.result ? options.resultSchema?.safeParse(run.result)?.data : undefined,

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 28, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add proper error handling for schema validation failures instead of silently ignoring them

    Add error handling for the schema validation result. Currently the code silently
    ignores validation failures when parsing the result. Consider throwing an error or
    providing validation status to the consumer.

    sdk-react/src/hooks/useRun.ts [198]

    -result: run?.result ? options.resultSchema?.safeParse(run.result)?.data : undefined,
    +result: run?.result && options.resultSchema 
    +  ? (() => {
    +      const parsed = options.resultSchema.safeParse(run.result);
    +      if (!parsed.success) {
    +        options.onError?.(new Error(`Result validation failed: ${parsed.error.message}`));
    +        return undefined;
    +      }
    +      return parsed.data;
    +    })()
    +  : undefined,
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a significant issue where validation failures are silently ignored, which could lead to runtime errors. The improved code properly handles validation failures and reports them through the existing error callback.

    8
    Security
    Add runtime validation for required authentication token to prevent silent failures

    The customAuthToken prop is marked as required in the type definition but there's no
    validation to ensure it's provided when using custom auth. Add runtime validation.

    sdk-react/demo/TestPage.tsx [6-13]

     type TestPageProps = {
       baseUrl?: string;
       apiSecret?: string;
       customAuthToken: string;
       clusterId: string;
       configId: string;
       initialPrompt?: string;
     };
     
    +export function TestPage(props: TestPageProps) {
    +  if (!props.customAuthToken) {
    +    throw new Error('customAuthToken is required when using custom authentication');
    +  }
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves security and reliability by adding runtime validation for the required customAuthToken. This prevents potential authentication failures that could occur silently at runtime.

    7
    General
    ✅ Remove unsafe type assertion and implement proper typing for schema validation
    Suggestion Impact:The commit removed the unsafe 'as any' type assertion and replaced it with proper schema validation using zod-to-json-schema

    code diff:

    -            resultSchema: options.resultSchema as any,
    +            ...(options.resultSchema
    +              ? { resultSchema: zodToJsonSchema(options.resultSchema) }
    +              : {}),

    The type assertion as any when passing resultSchema to createRun bypasses type
    safety. Consider defining proper types for the API contract to handle schema
    validation.

    sdk-react/src/hooks/useRun.ts [109]

    -resultSchema: options.resultSchema as any,
    +resultSchema: options.resultSchema?.shape ?? null,
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion addresses a type safety issue by removing the 'as any' type assertion. While the proposed solution improves type safety, it might need additional type definitions to be fully effective.

    6

    @nadeesha nadeesha merged commit 6c5e906 into main Dec 28, 2024
    28 checks passed
    @nadeesha nadeesha deleted the nadeesha/add-zod-validation-jsro branch December 28, 2024 08:22
    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.

    1 participant