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

Fix: drop dynamic field to support links in components #2

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

axe312ger
Copy link
Contributor

@axe312ger axe312ger commented Feb 20, 2025

User description

This simplifies our data structure and allows components to have lexical fields which relations to media and collections get auto populated based on the content within lexical.

Potentially breaking... so will be v0.2.0


PR Type

Enhancement, Documentation


Description

  • Refactored handling of media and links in Lexical editor.

    • Removed dynamic zones, introduced dedicated fields for media and links.
    • Enhanced parsing and extraction of media and links.
  • Simplified backend logic for managing relations to collections and media.

    • Introduced a default Links component for all collections.
    • Removed outdated components and streamlined component creation.
  • Updated README with detailed usage and integration instructions.

    • Added examples for media and link handling.
    • Clarified rendering strategies for media and links.

Changes walkthrough 📝

Relevant files
Enhancement
Input.tsx
Refactor media and link handling in Input component           

admin/src/components/Input.tsx

  • Refactored media and link handling logic.
  • Removed dynamic zone usage for links.
  • Added dedicated fields for media and links.
  • Improved error handling and parsing of Lexical documents.
  • +52/-36 
    LinkModal.tsx
    Update type for highlightText function                                     

    admin/src/components/LinkModal.tsx

    • Updated type for highlightText function.
    +1/-1     
    bootstrap.ts
    Simplify component management for media and links               

    server/src/bootstrap.ts

  • Removed dynamic zone components for media and links.
  • Introduced a default Links component for all collections.
  • Streamlined component creation and deletion logic.
  • +25/-64 
    lexical-search.ts
    Refactor search logic for Lexical editor                                 

    server/src/controllers/lexical-search.ts

  • Refactored search logic to handle all collections.
  • Removed dependency on dynamic zone components.
  • Enhanced query handling for content types.
  • +27/-20 
    Documentation
    README.md
    Update README with enhanced usage and integration details

    README.md

  • Updated usage instructions for media and link handling.
  • Added examples for rendering strategies.
  • Clarified integration details for Lexical documents.
  • +37/-17 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The error handling for failed API requests only shows alerts and console logs. Consider implementing proper error states and recovery mechanisms.

    } catch (err) {
      alert(
        'Failed to locate media used in the rich text. This may be due to a permission issue. Please contact your administrator or developer for assistance.'
      );
      console.error(err);
    }
    Missing Cleanup

    The code adds media and links but doesn't handle cleanup of removed items. As noted in the TODO comment, a disconnect array should be maintained.

      // @todo we probably have to maintain disconnect array as well to avoid issues on long term. No time right now for that ;)
    };
    Search Limitations

    The search functionality was simplified by removing filtered search capabilities. This may impact the ability to properly filter and search content.

    // @todo Reenable filtered search, either by config or by automatic detection of which link fields are available.
    // this got removed this was problematic with data structures that have sub components, the following code wont work with them:
    // const { model, field } = ctx.params;
    
    // const contentType = strapi.contentTypes[model];
    // const linksField = contentType.attributes[`${field}Links`];
    
    // console.dir({contentType}, {depth: null})
    // if (!linksField) {
    //   console.log('No links field found');
    //   ctx.body = [];
    //   return;
    // }

    Copy link

    qodo-merge-pro bot commented Feb 20, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Sanitize query parameters

    Add input validation for query parameters. The current code directly uses query
    parameters without sanitization, which could lead to injection vulnerabilities.

    server/src/controllers/lexical-search.ts [69-73]

     filters: {
       [fieldName]: {
    -    $contains: qs.q,
    +    $contains: typeof qs.q === 'string' ? qs.q.slice(0, 100) : '', // Sanitize and limit input length
       },
     },
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion addresses a critical security concern by adding input validation and length limiting for query parameters, preventing potential injection attacks.

    High
    Possible issue
    Validate pagination parameters

    Add error handling for empty or invalid pagination parameters. The current code
    assumes documentIds.size is always valid, which could lead to API errors if the
    size is 0 or exceeds server limits.

    admin/src/components/Input.tsx [143-146]

     pagination: {
       page: 1,
    -  pageSize: documentIds.size,
    +  pageSize: Math.min(Math.max(documentIds.size, 1), 100), // Ensure size is between 1 and 100
     },
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a potential API error by adding bounds checking for pagination size. This is important for preventing server errors and ensuring API stability.

    Medium
    Add type safety checks

    Add type checking for API response data before accessing properties. The current
    code assumes properties like 'name', 'title', etc. exist, which could cause
    runtime errors.

    admin/src/components/Input.tsx [155]

    -label: result['name'] || result['title'] || result['label'] || result['headline'],
    +label: typeof result === 'object' && result ? 
    +  (result['name'] || result['title'] || result['label'] || result['headline'] || 'Untitled') : 
    +  'Untitled',
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion adds important runtime type checking and provides a fallback value, preventing potential null/undefined errors when accessing API response properties.

    Medium
    • Update

    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