Skip to content

Conversation

@harsha509
Copy link
Member

@harsha509 harsha509 commented Jan 3, 2025

User description

fix js build with novnc

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix


Description

  • Fixed import path for RFB in LiveView.tsx.

  • Updated Bazel build configuration to remove outdated @novnc/novnc/core/rfb reference.


Changes walkthrough 📝

Relevant files
Bug fix
LiveView.tsx
Update `RFB` import path in `LiveView.tsx`                             

javascript/grid-ui/src/components/LiveView/LiveView.tsx

  • Updated RFB import path to @novnc/novnc/lib/rfb.
  • Ensured compatibility with the updated noVNC library structure.
  • +1/-1     
    BUILD.bazel
    Update Bazel build configuration for `noVNC`                         

    javascript/grid-ui/BUILD.bazel

  • Removed outdated @novnc/novnc/core/rfb from external dependencies.
  • Simplified Bazel build configuration for esbuild.
  • +0/-1     

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

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 3, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 3, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Include all required external dependencies in the Bazel build configuration to prevent build failures

    Add @novnc/novnc/lib/rfb to the external dependencies list since it's being imported
    in LiveView.tsx

    javascript/grid-ui/BUILD.bazel [76-82]

     external = [
         "fs",
         "module",
         "os",
         "path",
         "util",
    +    "@novnc/novnc/lib/rfb",
     ],
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical suggestion that prevents potential build failures by ensuring the newly imported @novnc/novnc/lib/rfb module is properly declared as an external dependency in the Bazel configuration, maintaining consistency with the import changes made in LiveView.tsx.

    9

    @VietND96
    Copy link
    Member

    VietND96 commented Jan 3, 2025

    @harsha509, thank you for your fix. It looks correct now.
    I have misunderstood between source https://github.com/novnc/noVNC/blob/master/core/rfb.js and the npm package https://www.npmjs.com/package/@novnc/novnc?activeTab=code
    So /lib/ is the correct one

    @codecov
    Copy link

    codecov bot commented Jan 3, 2025

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 58.61%. Comparing base (57f8398) to head (a12719c).
    Report is 1125 commits behind head on trunk.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #15014      +/-   ##
    ==========================================
    + Coverage   58.48%   58.61%   +0.13%     
    ==========================================
      Files          86       94       +8     
      Lines        5270     5997     +727     
      Branches      220      259      +39     
    ==========================================
    + Hits         3082     3515     +433     
    - Misses       1968     2223     +255     
    - Partials      220      259      +39     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @harsha509 harsha509 merged commit 27fbc2e into trunk Jan 3, 2025
    35 checks passed
    @harsha509 harsha509 deleted the js_build_fix branch January 3, 2025 07:50
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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.

    3 participants