Refactor-v-html-markdown.vue
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.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
Thank you for your contribution to GitLab. We believe that everyone can contribute and contributions like yours are what make GitLab great!
- Our Merge Request Coaches will ensure your contribution is reviewed in a timely manner*.
- If you haven't, please set up a
DANGER_GITLAB_API_TOKEN. - You can comment
@gitlab-bot label ~"group::"to add a group label. - After a few days, feel free to ask
@gitlab-bot helpor ping a Merge Request Coach. - Read more how to get help.
This message was generated automatically. You're welcome to improve it.
added Community contribution label
added Hackathon label
Resolved by Dheeraj Joshi@djadmin there are some tests failing, I did tasks mentioned in the issue page. Do tell me if I need to make any other modifications. Thank You.
- Last reply by Dheeraj Joshi
mentioned in issue gitlab-org/quality/triage-reports#4703 (closed)
requested review from @jannik_lehmann
assigned to @abhijeet007rocks8
Resolved by Dheeraj Joshi@djadmin I'm going through the unlabelled MR issue. Is there a particular group label for this?
- Last reply by Dheeraj Joshi
added frontend label
added devopscreate groupcode review labels
added maintenancerefactor label
- Resolved by Dheeraj Joshi
@abhijeet007rocks8
I have left 2 suggestions, please check them out, fixing them should also make to build go green/greener.
Ping me here if you have any questions on this!
- Last reply by Abhijeet Chatterjee
- Resolved by Dheeraj Joshi
Thanks! @abhijeet007rocks8
-
Please make sure this suggestion is taken care of as well: !70901 (comment 685094900)
-
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.
-
- Last reply by Jannik Lehmann
- Resolved by Dheeraj Joshi
@@abhijeet007rocks8 The Pipeline is still red due to some linting issues:
Please run
yarn run lint:prettierand commit the changes. - Last reply by Jannik Lehmann
added sectiondev label
5 Warnings 
The merge request title must contain at least 3 words. For more information, take a look at our Commit message guidelines. 6aeb477f: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. This merge request does not refer to an existing milestone. Reviewer roulette failed to load team data: Failed to read https://gitlab-org.gitlab.io/gitlab-roulette/roulette.json: 302 Found 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 
CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelogtrailer 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: truetrailer 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-reviewjob that generated this comment.Generated by
Thank you @abhijeet007rocks8 for quickly iterating over this. I've marked all the threads resolved, let's wait for the pipeline
@jannik_lehmann thanks for all the help with this MR, please take another look when you have a moment next week
Happy weekend everyone!
Bundle size analysis [beta]
This compares changes in bundle size for entry points between the commits 72148118 and 5ced201c
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 % 
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 % 
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,@mikegreilingor@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
Allure report
allure-report-publishergenerated test report for 5ced201c!review-qa-smoke:
- 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
- Last reply by Jannik Lehmann
-
@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!
- Resolved by Kushal Pandya
@jannik_lehmann, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, please start a new pipeline before merging.
For more info, please refer to the following links:
requested review from @kushalpandya
removed review request for @jannik_lehmann
changed milestone to %14.4
mentioned in commit 35f1082c
added workflowstaging-canary label
added workflowstaging label and removed workflowstaging-canary label
added workflowcanary label and removed workflowstaging label
added workflowproduction label and removed workflowcanary label
mentioned in issue #241888 (closed)
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
mentioned in merge request gitlab-com/www-gitlab-com!102279 (merged)
