Skip to content
Snippets Groups Projects

Refactor-v-html-markdown.vue

Merged Abhijeet Chatterjee requested to merge (removed):Remove-v-html-from-markdown.vue into master
All threads resolved!

What does this MR do and why?

Replaces dangerous v-html with our safe directive v-safe-html
Closes - #241888 (closed) Remove v-html from app/assets/javascripts/notebook/cells/markdown.vue

Screenshots or screen recordings

These are strongly recommended to assist reviewers and reduce the time to merge your change.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Abhijeet Chatterjee

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added 1 commit

    • 74947f3d - Apply suggestions on markdown.vue

    Compare with previous version

    • Resolved by Dheeraj Joshi

      Thanks! @abhijeet007rocks8

      1. Please make sure this suggestion is taken care of as well: !70901 (comment 685094900)

      2. One more thing that just catched my eye: This failing ci Job, https://gitlab.com/abhijeet007rocks8/gitlab/-/jobs/1615865050

      ERROR in ./notebook/cells/markdown.vue?vue&type=script&lang=js& (/builds/gitlab-org-forks/gitlab/node_modules/babel-loader/lib??ref--1!/builds/gitlab-org-forks/gitlab/node_modules/vue-loader/lib??vue-loader-options!./notebook/cells/markdown.vue?vue&type=script&lang=js&) 144:4-12 "export 'GlSafeHTMLDirective' (imported as 'SafeHtml') was not found in '@gitlab/ui'

      From what I can see, this import should work in this file, and I don't have an answer at hand why this is currently failing.

      This is a great question for the maintainer though.

      So let's get 1. out of the way make sure 2. is the only thing that's failing on the pipeline and we'll see what the maintainer says about the broken ci-job.

  • added 1 commit

    • 21803bf1 - Removing Glicon from markdown.vue

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • 5 Warnings
    :warning: The merge request title must contain at least 3 words. For more information, take a look at our Commit message guidelines.
    :warning: 6aeb477f: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
    :warning: This merge request does not refer to an existing milestone.
    :warning: Reviewer roulette failed to load team data: Failed to read https://gitlab-org.gitlab.io/gitlab-roulette/roulette.json: 302 Found
    :warning: You've made some app changes, but didn't add any tests.
    That's OK as long as you're refactoring existing code,
    but please consider adding any of the tooling, ~"tooling::pipelines", ~"tooling::workflow", documentation, QA labels.
    1 Message
    :book: CHANGELOG missing:

    If you want to create a changelog entry for GitLab FOSS, add the Changelog trailer to the commit message you want to add to the changelog.

    If you want to create a changelog entry for GitLab EE, also add the EE: true trailer to your commit message.

    If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

    Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    frontend No reviewer available No maintainer available

    To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

    If needed, you can retry the danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 3f18eb27 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Dheeraj Joshi resolved all threads

    resolved all threads

  • Thank you @abhijeet007rocks8 for quickly iterating over this. I've marked all the threads resolved, let's wait for the pipeline :fingers_crossed:

    @jannik_lehmann thanks for all the help with this MR, please take another look when you have a moment next week :slight_smile:

    Happy weekend everyone!

  • Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits 72148118 and 5ced201c

    :sparkles: Special assets

    Entrypoint / Name Size before Size after Diff Diff in percent
    average 3.23 MB 3.23 MB - 0.0 %
    mainChunk 1.89 MB 1.89 MB - 0.0 %

    :fearful: Significant Growth: 3

    Expand
    Entrypoint / Name Size before Size after Diff Diff in percent
    pages.projects.packages.packages.index 768.48 KB 920.82 KB +152.34 KB 19.8 %
    pages.groups.packages.index 23.23 KB 175.05 KB +151.83 KB 653.7 %
    pages.projects.packages.infrastructure_registry.show 1.15 MB 1.19 MB +33.47 KB 2.8 %

    :tada: Significant Reduction: 12

    Expand
    Entrypoint / Name Size before Size after Diff Diff in percent
    pages.groups.epics.new 1.61 MB 1.29 MB -327.81 KB -19.8 %
    pages.profiles.personal_access_tokens 341.81 KB 226.38 KB -115.43 KB -33.8 %
    pages.profiles 281.81 KB 166.56 KB -115.25 KB -40.9 %
    pages.profiles.slacks 289.6 KB 174.35 KB -115.25 KB -39.8 %
    pages.profiles.two_factor_auths 416.34 KB 302.43 KB -113.91 KB -27.4 %
    pages.profiles.preferences.show 295.88 KB 182.61 KB -113.27 KB -38.3 %
    pages.profiles.show 1.1 MB 1021.15 KB -109.39 KB -9.7 %
    pages.profiles.accounts.show 350.23 KB 309.94 KB -40.3 KB -11.5 %
    pages.profiles.billings 330.48 KB 290.18 KB -40.3 KB -12.2 %
    pages.profiles.keys 287.48 KB 247.18 KB -40.3 KB -14.0 %

    The table above is limited to 10 entries. Please look at the full report for more details


    Your MR has at least one entrypoint growing significantly (more > 1 KB or 2%). If you write new or extend existing features, this is expected and there is nothing to worry about.

    Please consider pinging someone from the FE Foundations (@dmishunov, @justin_ho, @mikegreiling or @nmezzopera) for review, if you are unsure about the size increase.

    Note: We do not have exact data for 72148118. So we have used data from: 901ddc8c.
    The intended commit has no webpack pipeline, so we chose the last commit with one before it.

    Please look at the full report for more details


    Read more about how this report works.

    Generated by :no_entry_sign: Danger

  • Allure report

    allure-report-publisher generated test report for 5ced201c!

    review-qa-smoke: :pencil: test report

    • Resolved by Kushal Pandya

      @abhijeet007rocks8 Thanks for staying on the ball on this!

      We're almost there, please apply the following patch:

      diff --git a/app/assets/javascripts/notebook/cells/markdown.vue b/app/assets/javascripts/notebook/cells/markdown.vue
      index 64510fe575d..073b27605bb 100644
      --- a/app/assets/javascripts/notebook/cells/markdown.vue
      +++ b/app/assets/javascripts/notebook/cells/markdown.vue
      @@ -154,7 +154,7 @@ export default {
             renderer.attachments = this.cell.attachments;
             renderer.relativeRawPath = this.relativeRawPath;
       
      -      return marked(this.cell.source.join('').replace(/\\/g, '\\\\')); 
      +      return marked(this.cell.source.join('').replace(/\\/g, '\\\\'));
           },
         },
         markdownConfig,

      This should fix the remaining linting Issues and we should have a green pipeline from there :fingers_crossed:

  • added 1 commit

    • 5ced201c - Remove trailing-space line 157

    Compare with previous version

  • Jannik Lehmann approved this merge request

    approved this merge request

  • yeah

    @abhijeet007rocks8 Thanks for staying on this! Pipeline is green, this is looking good!

    Happily approving this.

    @kushalpandya Can you maintainer-review this community-contribution? Thank you!

  • requested review from @kushalpandya

  • Jannik Lehmann removed review request for @jannik_lehmann

    removed review request for @jannik_lehmann

  • Kushal Pandya approved this merge request

    approved this merge request

  • Kushal Pandya changed milestone to %14.4

    changed milestone to %14.4

  • Kushal Pandya resolved all threads

    resolved all threads

  • merged

  • Kushal Pandya mentioned in commit 35f1082c

    mentioned in commit 35f1082c

  • added workflowcanary label and removed workflowstaging label

  • added workflowproduction label and removed workflowcanary label

  • mentioned in issue #241888 (closed)

  • Please register or sign in to reply
    Loading