diff options
32 files changed, 242 insertions, 153 deletions
diff --git a/app/assets/javascripts/behaviors/autosize.js b/app/assets/javascripts/behaviors/autosize.js index a5404539c17..181d841a068 100644 --- a/app/assets/javascripts/behaviors/autosize.js +++ b/app/assets/javascripts/behaviors/autosize.js @@ -1,13 +1,11 @@ import Autosize from 'autosize'; import { waitForCSSLoaded } from '~/helpers/startup_css_helper'; -document.addEventListener('DOMContentLoaded', () => { - waitForCSSLoaded(() => { - const autosizeEls = document.querySelectorAll('.js-autosize'); +waitForCSSLoaded(() => { + const autosizeEls = document.querySelectorAll('.js-autosize'); - Autosize(autosizeEls); - Autosize.update(autosizeEls); + Autosize(autosizeEls); + Autosize.update(autosizeEls); - autosizeEls.forEach((el) => el.classList.add('js-autosize-initialized')); - }); + autosizeEls.forEach((el) => el.classList.add('js-autosize-initialized')); }); diff --git a/app/graphql/types/base_enum.rb b/app/graphql/types/base_enum.rb index cbd45b46dd6..4d470aceca4 100644 --- a/app/graphql/types/base_enum.rb +++ b/app/graphql/types/base_enum.rb @@ -21,7 +21,7 @@ module Types graphql_name(enum_mod.name) if use_name description(enum_mod.description) if use_description - enum_mod.definition.each { |key, content| value(key.to_s.upcase, content) } + enum_mod.definition.each { |key, content| value(key.to_s.upcase, **content) } end def value(*args, **kwargs, &block) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 69a2efebb1f..4d748ac5bf8 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -254,7 +254,7 @@ module DiffHelper end def code_navigation_path(diffs) - Gitlab::CodeNavigationPath.new(merge_request.project, diffs.diff_refs&.head_sha) + Gitlab::CodeNavigationPath.new(merge_request.project, merge_request.diff_head_sha) end def conflicts diff --git a/app/serializers/diffs_entity.rb b/app/serializers/diffs_entity.rb index f573bbe8385..4bc6644a5cb 100644 --- a/app/serializers/diffs_entity.rb +++ b/app/serializers/diffs_entity.rb @@ -79,7 +79,9 @@ class DiffsEntity < Grape::Entity end expose :definition_path_prefix do |diffs| - project_blob_path(merge_request.project, diffs.diff_refs&.head_sha) + next unless merge_request.diff_head_sha + + project_blob_path(merge_request.project, merge_request.diff_head_sha) end def merge_request diff --git a/changelogs/unreleased/31064-ensure-diff-patch-size-limit-is-monitorable.yml b/changelogs/unreleased/31064-ensure-diff-patch-size-limit-is-monitorable.yml new file mode 100644 index 00000000000..59bdb1905be --- /dev/null +++ b/changelogs/unreleased/31064-ensure-diff-patch-size-limit-is-monitorable.yml @@ -0,0 +1,5 @@ +--- +title: Add `patch_hard_limit_bytes_hit` metric for monitoring diff patch size limit hits +merge_request: 52456 +author: +type: added diff --git a/changelogs/unreleased/feature-file-review-docs.yml b/changelogs/unreleased/feature-file-review-docs.yml new file mode 100644 index 00000000000..98e9d707b7a --- /dev/null +++ b/changelogs/unreleased/feature-file-review-docs.yml @@ -0,0 +1,6 @@ +--- +title: Enable local file reviews (marking files as viewed) by default and add documentation + for that feature +merge_request: 48976 +author: +type: added diff --git a/changelogs/unreleased/id-fix-no-route-error.yml b/changelogs/unreleased/id-fix-no-route-error.yml new file mode 100644 index 00000000000..85061ed731e --- /dev/null +++ b/changelogs/unreleased/id-fix-no-route-error.yml @@ -0,0 +1,5 @@ +--- +title: Fix viewing blobs for broken MRs +merge_request: 52483 +author: +type: fixed diff --git a/config/feature_flags/development/local_file_reviews.yml b/config/feature_flags/development/local_file_reviews.yml index 8b62b497c1c..4001f6ade72 100644 --- a/config/feature_flags/development/local_file_reviews.yml +++ b/config/feature_flags/development/local_file_reviews.yml @@ -1,8 +1,8 @@ --- name: local_file_reviews -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48976 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/51513 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/296674 -milestone: '13.8' +milestone: '13.9' type: development group: group::code review -default_enabled: false +default_enabled: true diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index 105c0a408ce..bd8236134a9 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/doc/administration/monitoring/prometheus/gitlab_metrics.md @@ -71,6 +71,7 @@ The following metrics are available: | `gitlab_transaction_event_etag_caching_resource_changed_total` | Counter | 9.4 | Counter for ETag cache miss - resource changed | `endpoint` | | `gitlab_transaction_event_fork_repository_total` | Counter | 9.4 | Counter for repository forks (RepositoryForkWorker). Only incremented when source repository exists | | | `gitlab_transaction_event_import_repository_total` | Counter | 9.4 | Counter for repository imports (RepositoryImportWorker) | | +| `gitlab_transaction_event_patch_hard_limit_bytes_hit_total` | Counter | 13.9 | Counter for diff patch size limit hits | | | `gitlab_transaction_event_push_branch_total` | Counter | 9.4 | Counter for all branch pushes | | | `gitlab_transaction_event_push_commit_total` | Counter | 9.4 | Counter for commits | `branch` | | `gitlab_transaction_event_push_tag_total` | Counter | 9.4 | Counter for tag pushes | | diff --git a/doc/ci/large_repositories/index.md b/doc/ci/large_repositories/index.md index 35b4a2de8f8..aff18b0889f 100644 --- a/doc/ci/large_repositories/index.md +++ b/doc/ci/large_repositories/index.md @@ -114,6 +114,11 @@ For example, if your project contains a large number of tags that your CI jobs d you could add [`--no-tags`](https://git-scm.com/docs/git-fetch#Documentation/git-fetch.txt---no-tags) to the extra flags to make your fetches faster and more compact. +Also in the case where you repository does _not_ contain a lot of +tags, `--no-tags` can [make a big difference in some +cases](https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/746). +If your CI builds do not depend on Git tags it is worth trying. + See the [`GIT_FETCH_EXTRA_FLAGS` documentation](../runners/README.md#git-fetch-extra-flags) for more information. diff --git a/doc/development/fe_guide/accessibility.md b/doc/development/fe_guide/accessibility.md index 92730e8139f..5ad1a701fac 100644 --- a/doc/development/fe_guide/accessibility.md +++ b/doc/development/fe_guide/accessibility.md @@ -9,11 +9,9 @@ info: To determine the technical writer assigned to the Stage/Group associated w ## Resources [Chrome Accessibility Developer Tools](https://github.com/GoogleChrome/accessibility-developer-tools) -are useful for testing for potential accessibility problems in GitLab. +assist with testing for potential accessibility problems in GitLab. -The [axe](https://www.deque.com/axe/) browser extension (available for [Firefox](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/) and [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd)) is -also a handy tool for running audits and getting feedback on markup, CSS and even potentially problematic color usages. +The [axe](https://www.deque.com/axe/) browser extension (available for [Firefox](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/) and [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd)) provides running audits and feedback on markup, CSS, and even potentially problematic color usages. Accessibility best-practices and more in-depth information are available on -[the Audit Rules page](https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules) for the Chrome Accessibility Developer Tools. The [Awesome Accessibility](https://github.com/brunopulis/awesome-a11y) list is also a -useful compilation of accessibility-related material. +[the Audit Rules page](https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules) for the Chrome Accessibility Developer Tools. The [Awesome Accessibility](https://github.com/brunopulis/awesome-a11y) list is a compilation of accessibility-related material. diff --git a/doc/development/fe_guide/architecture.md b/doc/development/fe_guide/architecture.md index 964837dc5f7..c51f99ca9d2 100644 --- a/doc/development/fe_guide/architecture.md +++ b/doc/development/fe_guide/architecture.md @@ -6,9 +6,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w # Architecture -When you are developing a new feature that requires architectural design, or if -you are changing the fundamental design of an existing feature, make sure it is -discussed with one of the Frontend Architecture Experts. +When developing a feature that requires architectural design, or changing the fundamental design of an existing feature, discuss it with a Frontend Architecture Expert. A Frontend Architect is an expert who makes high-level Frontend design decisions and decides on technical standards, including coding standards and frameworks. diff --git a/doc/development/fe_guide/axios.md b/doc/development/fe_guide/axios.md index cf5a4970c04..2d699b305ce 100644 --- a/doc/development/fe_guide/axios.md +++ b/doc/development/fe_guide/axios.md @@ -44,7 +44,7 @@ Advantages over [`spyOn()`](https://jasmine.github.io/api/edge/global.html#spyOn - no need to create response objects - does not allow call through (which we want to avoid) -- simple API to test error cases +- clear API to test error cases - provides `replyOnce()` to allow for different responses We have also decided against using [Axios interceptors](https://github.com/axios/axios#interceptors) because they are not suitable for mocking. diff --git a/doc/development/fe_guide/design_patterns.md b/doc/development/fe_guide/design_patterns.md index 784612682f8..d6b1f38b417 100644 --- a/doc/development/fe_guide/design_patterns.md +++ b/doc/development/fe_guide/design_patterns.md @@ -10,7 +10,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w When exactly one object is needed for a given task, prefer to define it as a `class` rather than as an object literal. Prefer also to explicitly restrict -instantiation, unless flexibility is important (e.g. for testing). +instantiation, unless flexibility is important (such as for testing). ```javascript // bad @@ -56,7 +56,7 @@ export default class MyThing { ## Manipulating the DOM in a JS Class When writing a class that needs to manipulate the DOM guarantee a container option is provided. -This is useful when we need that class to be instantiated more than once in the same page. +This can be used when we need that class to be instantiated more than once in the same page. Bad: diff --git a/doc/development/fe_guide/development_process.md b/doc/development/fe_guide/development_process.md index d122459f51c..b85ed4da442 100644 --- a/doc/development/fe_guide/development_process.md +++ b/doc/development/fe_guide/development_process.md @@ -12,9 +12,9 @@ You can find more about the organization of the frontend team in the [handbook]( The idea is to remind us about specific topics during the time we build a new feature or start something. This is a common practice in other industries (like pilots) that also use standardized checklists to reduce problems early on. -Copy the content over to your issue or merge request and if something doesn't apply simply remove it from your current list. +Copy the content over to your issue or merge request and if something doesn't apply, remove it from your current list. -This checklist is intended to help us during development of bigger features/refactorings, it's not a "use it always and every point always matches" list. +This checklist is intended to help us during development of bigger features/refactorings. It is not a "use it always and every point always matches" list. Please use your best judgment when to use it and please contribute new points through merge requests if something comes to your mind. @@ -77,7 +77,7 @@ With the purpose of being [respectful of others' time](https://about.gitlab.com/ - includes tests - includes a changelog entry (when necessary) - Before assigning to a maintainer, assign to a reviewer. -- If you assigned a merge request or pinged someone directly, be patient because we work in different timezones and asynchronously. Unless the merge request is urgent (like fixing a broken master), please don't DM or reassign the merge request before waiting for a 24-hour window. +- If you assigned a merge request or pinged someone directly, be patient because we work in different timezones and asynchronously. Unless the merge request is urgent (like fixing a broken default branch), please don't DM or reassign the merge request before waiting for a 24-hour window. - If you have a question regarding your merge request/issue, make it on the merge request/issue. When we DM each other, we no longer have a SSOT and [no one else is able to contribute](https://about.gitlab.com/handbook/values/#public-by-default). - When you have a big **Draft** merge request with many changes, you're advised to get the review started before adding/removing significant code. Make sure it is assigned well before the release cut-off, as the reviewer(s)/maintainer(s) would always prioritize reviewing finished MRs before the **Draft** ones. - Make sure to remove the `Draft:` title before the last round of review. diff --git a/doc/development/fe_guide/editor_lite.md b/doc/development/fe_guide/editor_lite.md index 47ef85d8737..3655f86c07c 100644 --- a/doc/development/fe_guide/editor_lite.md +++ b/doc/development/fe_guide/editor_lite.md @@ -65,7 +65,7 @@ The editor follows the same public API as [provided by Monaco editor](https://mi 1. Editor's loading state. -Editor Lite comes with the loading state built-in, making spinners and loaders rarely needed in HTML. To benefit the built-in loading state, set the `data-editor-loading` property on the HTML element that is supposed to contain the editor. Editor Lite will show the loader automatically while it's bootstrapping. +Editor Lite comes with the loading state built-in, making spinners and loaders rarely needed in HTML. To benefit the built-in loading state, set the `data-editor-loading` property on the HTML element that is supposed to contain the editor. Editor Lite shows the loader automatically while it's bootstrapping.  1. Update syntax highlighting if the filename changes. @@ -89,7 +89,7 @@ form.addEventListener('submit', () => { 1. Performance -Even though Editor Lite itself is extremely slim, it still depends on Monaco editor. Monaco is not an easily tree-shakeable module. Hence, every time you add Editor Lite to a view, the JavaScript bundle's size significantly increases, affecting your view's loading performance. To avoid that, it is recommended to import the editor on demand on those views where it is not 100% certain that the editor will be used. Or if the editor is a secondary element of the view. Loading Editor Lite on demand is no different from loading any other module: +Even though Editor Lite itself is extremely slim, it still depends on Monaco editor. Monaco is not an easily tree-shakeable module. Hence, every time you add Editor Lite to a view, the JavaScript bundle's size significantly increases, affecting your view's loading performance. It is recommended to import the editor on demand on those views where it is not 100% certain that the editor is needed. Or if the editor is a secondary element of the view. Loading Editor Lite on demand is no different from loading any other module: ```javascript someActionFunction() { @@ -109,8 +109,8 @@ which would not depend on any particular group. Even though the Editor Lite's co [Create::Editor FE Team](https://about.gitlab.com/handbook/engineering/development/dev/create-editor/), the main functional elements — extensions — can be owned by any group. Editor Lite extensions' main idea is that the core of the editor remains very slim and stable. At the same time, whatever new functionality -is needed can be added as an extension to this core, without touching the core itself. It allows any group -to build and own any new editing functionality without being afraid of it being broken or overridden with +is needed can be added as an extension to this core, without touching the core itself. Any group is allowed +to build and own new editing functionality without being afraid of it being broken or overridden with the Editor Lite changes. Structurally, the complete implementation of Editor Lite could be presented as the following diagram: @@ -145,7 +145,7 @@ Important things to note here: ### Using an existing extension -Adding an extension to Editor Lite's instance is simple: +Adding an extension to Editor Lite's instance requires the following steps: ```javascript import EditorLite from '~/editor/editor_lite'; @@ -159,7 +159,7 @@ editor.use(MyExtension); ### Creating an extension -Let's create our first Editor Lite extension. As aforementioned, extensions are ES6 modules exporting the simple `Object` that is used to extend Editor Lite's functionality. As the most straightforward test, let's create an extension that extends Editor Lite with a new function that, when called, will output editor's content in `alert`. +Let's create our first Editor Lite extension. Extensions are ES6 modules exporting a basic `Object` that is used to extend Editor Lite's functionality. As a test, let's create an extension that extends Editor Lite with a new function that, when called, outputs editor's content in `alert`. `~/my_folder/my_fancy_extension.js:` diff --git a/doc/development/fe_guide/emojis.md b/doc/development/fe_guide/emojis.md index 7151e2ffeee..2dedbc8f19d 100644 --- a/doc/development/fe_guide/emojis.md +++ b/doc/development/fe_guide/emojis.md @@ -25,7 +25,7 @@ when your platform does not support it. - `app/assets/images/emoji.png` - `app/assets/images/emoji@2x.png` 1. Ensure you see new individual images copied into `app/assets/images/emoji/` - 1. Ensure you can see the new emojis and their aliases in the GFM Autocomplete + 1. Ensure you can see the new emojis and their aliases in the GitLab Flavored Markdown (GFM) Autocomplete 1. Ensure you can see the new emojis and their aliases in the award emoji menu 1. You might need to add new emoji Unicode support checks and rules for platforms that do not support a certain emoji and we need to fallback to an image. diff --git a/doc/development/fe_guide/frontend_faq.md b/doc/development/fe_guide/frontend_faq.md index 9612f604b56..772f1672293 100644 --- a/doc/development/fe_guide/frontend_faq.md +++ b/doc/development/fe_guide/frontend_faq.md @@ -21,7 +21,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w ## FAQ -### 1. How do I find the Rails route for a page? +### 1. How does one find the Rails route for a page? #### Check the 'page' data attribute @@ -36,7 +36,7 @@ Find here the [source code setting the attribute](https://gitlab.com/gitlab-org/ #### Rails routes -The `rake routes` command can be used to list all the routes available in the application, piping the output into `grep`, we can perform a search through the list of available routes. +The `rake routes` command can be used to list all the routes available in the application. Piping the output into `grep`, we can perform a search through the list of available routes. The output includes the request types available, route parameters and the relevant controller. ```shell @@ -46,13 +46,13 @@ bundle exec rake routes | grep "issues" ### 2. `modal_copy_button` vs `clipboard_button` The `clipboard_button` uses the `copy_to_clipboard.js` behavior, which is -initialized on page load, so if there are vue-based clipboard buttons that -don't exist at page load (such as ones in a `GlModal`), they do not have the +initialized on page load. Vue clipboard buttons that +don't exist at page load (such as ones in a `GlModal`) do not have click handlers associated with the clipboard package. -`modal_copy_button` was added that manages an instance of the +`modal_copy_button` manages an instance of the [`clipboard` plugin](https://www.npmjs.com/package/clipboard) specific to -the instance of that component, which means that clipboard events are +the instance of that component. This means that clipboard events are bound on mounting and destroyed when the button is, mitigating the above issue. It also has bindings to a particular container or modal ID available, to work with the focus trap created by our GlModal. @@ -60,7 +60,7 @@ available, to work with the focus trap created by our GlModal. ### 3. A `gitlab-ui` component not conforming to [Pajamas Design System](https://design.gitlab.com/) Some [Pajamas Design System](https://design.gitlab.com/) components implemented in -`gitlab-ui` do not conform with the design system specs because they lack some +`gitlab-ui` do not conform with the design system specs. This is because they lack some planned features or are not correctly styled yet. In the Pajamas website, a banner on top of the component examples indicates that: @@ -77,18 +77,17 @@ It makes codebase unified and more comfortable to maintain/refactor in the futur Ensure a [Product Designer](https://about.gitlab.com/company/team/?department=ux-department) reviews the use of the non-conforming component as part of the MR review. Make a -follow up issue and attach it to the component implementation epic found within the +follow up issue and attach it to the component implementation epic found in the [Components of Pajamas Design System epic](https://gitlab.com/groups/gitlab-org/-/epics/973). ### 4. My submit form button becomes disabled after submitting -If you are using a submit button inside a form and you attach an `onSubmit` event listener on the form element, [this piece of code](https://gitlab.com/gitlab-org/gitlab/blob/794c247a910e2759ce9b401356432a38a4535d49/app/assets/javascripts/main.js#L225) adds a `disabled` class selector to the submit button when the form is submitted. -To avoid this behavior, add the class `js-no-auto-disable` to the button. +A Submit button inside of a form attaches an `onSubmit` event listener on the form element. [This code](https://gitlab.com/gitlab-org/gitlab/blob/794c247a910e2759ce9b401356432a38a4535d49/app/assets/javascripts/main.js#L225) adds a `disabled` class selector to the submit button when the form is submitted. To avoid this behavior, add the class `js-no-auto-disable` to the button. -### 5. Should I use a full URL (i.e. `gon.gitlab_url`) or a full path (i.e. `gon.relative_url_root`) when referencing backend endpoints? +### 5. Should one use a full URL (for example `gon.gitlab_url`) or a full path (for example `gon.relative_url_root`) when referencing backend endpoints? -It's preferred to use a **full path** over a **full URL** because the URL uses the hostname configured with -GitLab which may not match the request. This causes [CORS issues like this Web IDE one](https://gitlab.com/gitlab-org/gitlab/-/issues/36810). +It's preferred to use a **full path** over a **full URL**. This is because the URL uses the hostname configured with +GitLab which may not match the request. This causes [cross-origin resource sharing issues like this Web IDE example](https://gitlab.com/gitlab-org/gitlab/-/issues/36810). Example: @@ -117,7 +116,7 @@ Example: ### 6. How should the Frontend reference Backend paths? -We prefer not to add extra coupling by hardcoding paths. If possible, +We prefer not to add extra coupling by hard-coding paths. If possible, add these paths as data attributes to the DOM element being referenced in the JavaScript. Example: @@ -153,7 +152,7 @@ export const fetchFoos = ({ state }) => { }; ``` -### 7. How can I test the production build locally? +### 7. How can one test the production build locally? Sometimes it's necessary to test locally what the frontend production build would produce, to do so the steps are: @@ -161,7 +160,7 @@ Sometimes it's necessary to test locally what the frontend production build woul 1. Open `gitlab.yaml` located in your `gitlab` installation folder, scroll down to the `webpack` section and change `dev_server` to `enabled: false`. 1. Run `yarn webpack-prod && gdk restart rails-web`. -The production build takes a few minutes to be completed; any code changes at this point are +The production build takes a few minutes to be completed. Any code changes at this point are displayed only after executing the item 3 above again. To return to the normal development mode: @@ -176,8 +175,8 @@ To return to the normal development mode: > [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/28837) in GitLab 12.8. GitLab has enabled the Babel `preset-env` option -[`useBuiltIns: 'usage'`](https://babeljs.io/docs/en/babel-preset-env#usebuiltins-usage), -which adds the appropriate `core-js` polyfills once for each JavaScript feature +[`useBuiltIns: 'usage'`](https://babeljs.io/docs/en/babel-preset-env#usebuiltins-usage). +This adds the appropriate `core-js` polyfills once for each JavaScript feature we're using that our target browsers don't support. You don't need to add `core-js` polyfills manually. diff --git a/doc/development/fe_guide/graphql.md b/doc/development/fe_guide/graphql.md index 157e2f042e7..7e581939346 100644 --- a/doc/development/fe_guide/graphql.md +++ b/doc/development/fe_guide/graphql.md @@ -25,7 +25,7 @@ info: "See the Technical Writers assigned to Development Guidelines: https://abo - A real-life example of implementing a frontend feature in GitLab using GraphQL - [🎬 History of client-side GraphQL at GitLab](https://www.youtube.com/watch?v=mCKRJxvMnf0) (video) Illya Klymov and Natalia Tepluhina - [🎬 From Vuex to Apollo](https://www.youtube.com/watch?v=9knwu87IfU8) (video) by Natalia Tepluhina - - A useful overview of when Apollo might be a better choice than Vuex, and how one could go about the transition + - An overview of when Apollo might be a better choice than Vuex, and how one could go about the transition - [🛠 Vuex -> Apollo Migration: a proof-of-concept project](https://gitlab.com/ntepluhina/vuex-to-apollo/blob/master/README.md) - A collection of examples that show the possible approaches for state management with Vue+GraphQL+(Vuex or Apollo) apps @@ -34,7 +34,7 @@ info: "See the Technical Writers assigned to Development Guidelines: https://abo We use [Apollo](https://www.apollographql.com/) (specifically [Apollo Client](https://www.apollographql.com/docs/react/)) and [Vue Apollo](https://github.com/vuejs/vue-apollo) when using GraphQL for frontend development. -If you are using GraphQL within a Vue application, the [Usage in Vue](#usage-in-vue) section +If you are using GraphQL in a Vue application, the [Usage in Vue](#usage-in-vue) section can help you learn how to integrate Vue Apollo. For other use cases, check out the [Usage outside of Vue](#usage-outside-of-vue) section. @@ -76,7 +76,7 @@ Our GraphQL API can be explored via GraphiQL at your instance's where needed. You can check all existing queries and mutations on the right side -of GraphiQL in its **Documentation explorer**. It's also possible to +of GraphiQL in its **Documentation explorer**. You can also write queries and mutations directly on the left tab and check their execution by clicking **Execute query** button on the top left: @@ -93,8 +93,8 @@ Default client accepts two parameters: `resolvers` and `config`. - `resolvers` parameter is created to accept an object of resolvers for [local state management](#local-state-with-apollo) queries and mutations - `config` parameter takes an object of configuration settings: - `cacheConfig` field accepts an optional object of settings to [customize Apollo cache](https://www.apollographql.com/docs/react/caching/cache-configuration/#configuring-the-cache) - - `baseUrl` allows us to pass a URL for GraphQL endpoint different from our main endpoint (i.e.`${gon.relative_url_root}/api/graphql`) - - `assumeImmutableResults` (set to `false` by default) - this setting, when set to `true`, will assume that every single operation on updating Apollo Cache is immutable. It also sets `freezeResults` to `true`, so any attempt on mutating Apollo Cache will throw a console warning in development environment. Please ensure you're following the immutability pattern on cache update operations before setting this option to `true`. + - `baseUrl` allows us to pass a URL for GraphQL endpoint different from our main endpoint (for example, `${gon.relative_url_root}/api/graphql`) + - `assumeImmutableResults` (set to `false` by default) - this setting, when set to `true`, assumes that every single operation on updating Apollo Cache is immutable. It also sets `freezeResults` to `true`, so any attempt on mutating Apollo Cache throws a console warning in development environment. Please ensure you're following the immutability pattern on cache update operations before setting this option to `true`. - `fetchPolicy` determines how you want your component to interact with the Apollo cache. Defaults to "cache-first". ## GraphQL Queries @@ -139,7 +139,7 @@ fragment DesignItem on Design { ``` More about fragments: -[GraphQL Docs](https://graphql.org/learn/queries/#fragments) +[GraphQL documentation](https://graphql.org/learn/queries/#fragments) ## Global IDs @@ -157,7 +157,7 @@ const primaryKeyId = getIdFromGraphQLId(data.id); ## Immutability and cache updates -From Apollo version 3.0.0 all the cache updates need to be immutable; it needs to be replaced entirely +From Apollo version 3.0.0 all the cache updates need to be immutable. It needs to be replaced entirely with a **new and updated** object. To facilitate the process of updating the cache and returning the new object we use the library [Immer](https://immerjs.github.io/immer/docs/introduction). @@ -184,10 +184,10 @@ client.writeQuery({ ``` As shown in the code example by using `produce`, we can perform any kind of direct manipulation of the -`draftState`. Besides, `immer` guarantees that a new state which includes the changes to `draftState` will be generated. +`draftState`. Besides, `immer` guarantees that a new state which includes the changes to `draftState` is generated. Finally, to verify whether the immutable cache update is working properly, we need to change -`assumeImmutableResults` to `true` in the default client configuration (see [Apollo Client](#apollo-client) for more information). +`assumeImmutableResults` to `true` in the default client configuration. See [Apollo Client](#apollo-client) for more information. If everything is working properly `assumeImmutableResults` should remain set to `true`. @@ -259,11 +259,11 @@ query User { } ``` -Along with creating local data, we can also extend existing GraphQL types with `@client` fields. This is extremely useful when we need to mock an API responses for fields not yet added to our GraphQL API. +Along with creating local data, we can also extend existing GraphQL types with `@client` fields. This is extremely helpful when we need to mock an API response for fields not yet added to our GraphQL API. #### Mocking API response with local Apollo cache -Using local Apollo Cache is handy when we have a need to mock some GraphQL API responses, queries or mutations locally (e.g. when they're still not added to our actual API). +Using local Apollo Cache is helpful when we have a need to mock some GraphQL API responses, queries, or mutations locally (such as when they're still not added to our actual API). For example, we have a [fragment](#fragments) on `DesignVersion` used in our queries: @@ -274,7 +274,7 @@ fragment VersionListItem on DesignVersion { } ``` -We need to fetch also version author and the 'created at' property to display them in the versions dropdown but these changes are still not implemented in our API. We can change the existing fragment to get a mocked response for these new fields: +We also need to fetch the version author and the `created at` property to display in the versions dropdown. But, these changes are still not implemented in our API. We can change the existing fragment to get a mocked response for these new fields: ```javascript fragment VersionListItem on DesignVersion { @@ -288,7 +288,7 @@ fragment VersionListItem on DesignVersion { } ``` -Now Apollo will try to find a _resolver_ for every field marked with `@client` directive. Let's create a resolver for `DesignVersion` type (why `DesignVersion`? because our fragment was created on this type). +Now Apollo tries to find a _resolver_ for every field marked with `@client` directive. Let's create a resolver for `DesignVersion` type (why `DesignVersion`? because our fragment was created on this type). ```javascript // resolvers.js @@ -319,13 +319,13 @@ import resolvers from './graphql/resolvers'; const defaultClient = createDefaultClient(resolvers); ``` -For each attempt to fetch a version, our client will fetch `id` and `sha` from the remote API endpoint and will assign our hardcoded values to the `author` and `createdAt` version properties. With this data, frontend developers are able to work on their UI without being blocked by backend. When the actual response is added to the API, our custom local resolver can be removed and the only change to the query/fragment is to remove the `@client` directive. +For each attempt to fetch a version, our client fetches `id` and `sha` from the remote API endpoint. It then assigns our hardcoded values to the `author` and `createdAt` version properties. With this data, frontend developers are able to work on their UI without being blocked by backend. When the response is added to the API, our custom local resolver can be removed. The only change to the query/fragment is to remove the `@client` directive. Read more about local state management with Apollo in the [Vue Apollo documentation](https://vue-apollo.netlify.app/guide/local-state.html#local-state). ### Using with Vuex -When Apollo Client is used within Vuex and fetched data is stored in the Vuex store, there is no need to keep Apollo Client cache enabled. Otherwise we would have data from the API stored in two places - Vuex store and Apollo Client cache. With Apollo's default settings, a subsequent fetch from the GraphQL API could result in fetching data from Apollo cache (in the case where we have the same query and variables). To prevent this behavior, we need to disable Apollo Client cache by passing a valid `fetchPolicy` option to its constructor: +When the Apollo Client is used in Vuex and fetched data is stored in the Vuex store, the Apollo Client cache does not need to be enabled. Otherwise we would have data from the API stored in two places - Vuex store and Apollo Client cache. With Apollo's default settings, a subsequent fetch from the GraphQL API could result in fetching data from Apollo cache (in the case where we have the same query and variables). To prevent this behavior, we need to disable Apollo Client cache by passing a valid `fetchPolicy` option to its constructor: ```javascript import fetchPolicies from '~/graphql_shared/fetch_policy_constants'; @@ -390,9 +390,9 @@ the appropriate issue. #### Feature flags in queries -Sometimes it may be useful to have an entity in the GraphQL query behind a feature flag. -For example, when working on a feature where the backend has already been merged but the frontend -hasn't you might want to put the GraphQL entity behind a feature flag to allow for smaller +Sometimes it may be helpful to have an entity in the GraphQL query behind a feature flag. +One example is working on a feature where the backend has already been merged but the frontend +has not. In this case, you may consider putting the GraphQL entity behind a feature flag to allow smaller merge requests to be created and merged. To do this we can use the `@include` directive to exclude an entity if the `if` statement passes. @@ -405,7 +405,7 @@ query getAuthorData($authorNameEnabled: Boolean = false) { ``` Then in the Vue (or JavaScript) call to the query we can pass in our feature flag. This feature -flag will need to be already setup correctly. See the [feature flag documentation](../feature_flags/development.md) +flag needs to be already set up correctly. See the [feature flag documentation](../feature_flags/development.md) for the correct way to do this. ```javascript @@ -519,7 +519,7 @@ Note that we are using the [`pageInfo.fragment.graphql`](https://gitlab.com/gitl #### Using `fetchMore` method in components -This approach makes sense to use with user-handled pagination (e.g. when the scrolls to fetch more data or explicitly clicks a "Next Page"-button). +This approach makes sense to use with user-handled pagination. For example, when the scrolling to fetch more data or explicitly clicking a **Next Page** button. When we need to fetch all the data initially, it is recommended to use [a (non-smart) query, instead](#using-a-recursive-query-in-components). When making an initial fetch, we usually want to start a pagination from the beginning. @@ -529,7 +529,7 @@ In this case, we can either: - Pass `null` explicitly to `after`. After data is fetched, we can use the `update`-hook as an opportunity [to customize -the data that is set in the Vue component property](https://apollo.vuejs.org/api/smart-query.html#options), getting a hold of the `pageInfo` object among other data. +the data that is set in the Vue component property](https://apollo.vuejs.org/api/smart-query.html#options). This allows us to get a hold of the `pageInfo` object among other data. In the `result`-hook, we can inspect the `pageInfo` object to see if we need to fetch the next page. Note that we also keep a `requestCount` to ensure that the application @@ -611,8 +611,8 @@ fetchNextPage(endCursor) { When it is necessary to fetch all paginated data initially an Apollo query can do the trick for us. If we need to fetch the next page based on user interactions, it is recommend to use a [`smartQuery`](https://apollo.vuejs.org/api/smart-query.html) along with the [`fetchMore`-hook](#using-fetchmore-method-in-components). -When the query resolves we can update the component data and inspect the `pageInfo` object -to see if we need to fetch the next page, i.e. call the method recursively. +When the query resolves we can update the component data and inspect the `pageInfo` object. This allows us +to see if we need to fetch the next page, calling the method recursively. Note that we also keep a `requestCount` to ensure that the application does not keep requesting the next page, indefinitely. @@ -684,7 +684,7 @@ or [`.writeQuery()`](https://www.apollographql.com/docs/react/v2/api/apollo-clie This can be tedious and counter-intuitive. To make it easier to deal with cached paginated queries, Apollo provides the `@connection` directive. -The directive accepts a `key` parameter that will be used as a static key when caching the data. +The directive accepts a `key` parameter that is used as a static key when caching the data. You'd then be able to retrieve the data without providing any pagination-specific variables. Here's an example of a query using the `@connection` directive: @@ -711,7 +711,7 @@ query DastSiteProfiles($fullPath: ID!, $after: String, $before: String, $first: } ``` -In this example, Apollo will store the data with the stable `dastSiteProfiles` cache key. +In this example, Apollo stores the data with the stable `dastSiteProfiles` cache key. To retrieve that data from the cache, you'd then only need to provide the `$fullPath` variable, omitting pagination-specific variables like `after` or `before`: @@ -729,9 +729,9 @@ Read more about the `@connection` directive in [Apollo's documentation](https:// ### Managing performance -The Apollo client will batch queries by default. This means that if you have 3 queries defined, -Apollo will group them into one request, send the single request off to the server and only -respond once all 3 queries have completed. +The Apollo client batches queries by default. Given 3 deferred queries, +Apollo groups them into one request, sends the single request to the server, and +responds after all 3 queries have completed. If you need to have queries sent as individual requests, additional context can be provided to tell Apollo to do this. @@ -753,7 +753,7 @@ export default { #### Mocking response as component data -With [Vue test utils](https://vue-test-utils.vuejs.org/) it is easy to quickly test components that +With [Vue test utils](https://vue-test-utils.vuejs.org/) one can quickly test components that fetch GraphQL queries. The simplest way is to use `shallowMount` and then set the data on the component @@ -769,7 +769,7 @@ it('tests apollo component', () => { #### Testing loading state -If we need to test how our component renders when results from the GraphQL API are still loading, we can mock a loading state into respective Apollo queries/mutations: +To test how a component renders when results from the GraphQL API are still loading, mock a loading state into respective Apollo queries/mutations: ```javascript function createComponent({ @@ -867,9 +867,9 @@ it('calls mutation on submitting form ', () => { To test the logic of Apollo cache updates, we might want to mock an Apollo Client in our unit tests. We use [`mock-apollo-client`](https://www.npmjs.com/package/mock-apollo-client) library to mock Apollo client and [`createMockApollo` helper](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/frontend/__helpers__/mock_apollo_helper.js) we created on top of it. -To separate tests with mocked client from 'usual' unit tests, it's recommended to create an additional factory and pass the created `mockApollo` as an option to the `createComponent`-factory. This way we only create Apollo Client instance when it's necessary. +To separate tests with mocked client from 'usual' unit tests, create an additional factory and pass the created `mockApollo` as an option to the `createComponent`-factory. This way we only create Apollo Client instance when it's necessary. -We need to inject `VueApollo` to the Vue local instance and, likewise, it is recommended to call `localVue.use()` within `createMockApolloProvider()` to only load it when it is necessary. +We need to inject `VueApollo` to the Vue local instance and, likewise, it is recommended to call `localVue.use()` in `createMockApolloProvider()` to only load it when it is necessary. ```javascript import VueApollo from 'vue-apollo'; @@ -911,7 +911,7 @@ describe('Some component', () => { }); ``` -Within `createMockApolloProvider`-factory, we need to define an array of _handlers_ for every query or mutation: +In the `createMockApolloProvider`-factory, we need to define an array of _handlers_ for every query or mutation: ```javascript import getDesignListQuery from '~/design_management/graphql/queries/get_design_list.query.graphql'; @@ -1301,9 +1301,9 @@ describe('My Index test with `createMockApollo`', () => { ## Handling errors -The GitLab GraphQL mutations currently have two distinct error modes: [Top-level](#top-level-errors) and [errors-as-data](#errors-as-data). +The GitLab GraphQL mutations have two distinct error modes: [Top-level](#top-level-errors) and [errors-as-data](#errors-as-data). -When utilising a GraphQL mutation, we must consider handling **both of these error modes** to ensure that the user receives the appropriate feedback when an error occurs. +When utilising a GraphQL mutation, consider handling **both of these error modes** to ensure that the user receives the appropriate feedback when an error occurs. ### Top-level errors @@ -1311,13 +1311,13 @@ These errors are located at the "top level" of a GraphQL response. These are non #### Handling top-level errors -Apollo is aware of top-level errors, so we are able to leverage Apollo's various error-handling mechanisms to handle these errors (e.g. handling Promise rejections after invoking the [`mutate`](https://www.apollographql.com/docs/react/api/core/ApolloClient/#ApolloClient.mutate) method, or handling the `error` event emitted from the [`ApolloMutation`](https://apollo.vuejs.org/api/apollo-mutation.html#events) component). +Apollo is aware of top-level errors, so we are able to leverage Apollo's various error-handling mechanisms to handle these errors. For example, handling Promise rejections after invoking the [`mutate`](https://www.apollographql.com/docs/react/api/core/ApolloClient/#ApolloClient.mutate) method, or handling the `error` event emitted from the [`ApolloMutation`](https://apollo.vuejs.org/api/apollo-mutation.html#events) component. Because these errors are not intended for users, error messages for top-level errors should be defined client-side. ### Errors-as-data -These errors are nested within the `data` object of a GraphQL response. These are recoverable errors that, ideally, can be presented directly to the user. +These errors are nested in the `data` object of a GraphQL response. These are recoverable errors that, ideally, can be presented directly to the user. #### Handling errors-as-data @@ -1333,7 +1333,7 @@ mutation createNoteMutation($input: String!) { } ``` -Now, when we commit this mutation and errors occur, the response will include `errors` for us to handle: +Now, when we commit this mutation and errors occur, the response includes `errors` for us to handle: ```javascript { @@ -1366,7 +1366,7 @@ When [using Vuex](#using-with-vuex), disable the cache when: - The data is being cached elsewhere - The use case does not need caching -if the data is being cached elsewhere, or if there is simply no need for it for the given use case. +if the data is being cached elsewhere, or if there is no need for it for the given use case. ```javascript import createDefaultClient from '~/lib/graphql'; @@ -1466,7 +1466,7 @@ for your application. To add GraphQL startup calls, we use `add_page_startup_graphql_call` helper where the first parameter is a path to the query, the second one is an object containing query variables. Path to the query is relative to `app/graphql/queries` folder: for example, if we need a -`app/graphql/queries/repository/files.query.graphql` query, the path will be +`app/graphql/queries/repository/files.query.graphql` query, the path is `repository/files`. ```yaml diff --git a/doc/development/fe_guide/icons.md b/doc/development/fe_guide/icons.md index af587a31bbb..852de1f98b8 100644 --- a/doc/development/fe_guide/icons.md +++ b/doc/development/fe_guide/icons.md @@ -100,7 +100,7 @@ by the examples that follow: - `container` (optional): wraps the loading icon in a container, which centers the loading icon using the `text-center` CSS property. - `color` (optional): either `orange` (default), `light`, or `dark`. - `size` (optional): either `sm` (default), `md`, `lg`, or `xl`. -- `css_class` (optional): defaults to an empty string, but can be useful for utility classes to fine-tune alignment or spacing. +- `css_class` (optional): defaults to an empty string, but can be used for utility classes to fine-tune alignment or spacing. **Example 1:** @@ -164,8 +164,8 @@ export default { ## SVG Illustrations -Please use from now on for any SVG based illustrations simple `img` tags to show an illustration by simply using either `image_tag` or `image_path` helpers. -Please use the class `svg-content` around it to ensure nice rendering. +From now on, use `img` tags to display any SVG based illustrations with either `image_tag` or `image_path` helpers. +Using the class `svg-content` around it ensures nice rendering. ### Usage in HAML/Rails diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index 84c1623f8c0..65b2f6f9718 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -11,7 +11,7 @@ across the GitLab frontend team. ## Overview -GitLab is built on top of [Ruby on Rails](https://rubyonrails.org) using [Haml](https://haml.info/) and also a JavaScript based Frontend with [Vue.js](https://vuejs.org). +GitLab is built on top of [Ruby on Rails](https://rubyonrails.org). It uses [Haml](https://haml.info/) and a JavaScript0based frontend with [Vue.js](https://vuejs.org). Be wary of [the limitations that come with using Hamlit](https://github.com/k0kubun/hamlit/blob/master/REFERENCE.md#limitations). We also use [SCSS](https://sass-lang.com) and plain JavaScript with modern ECMAScript standards supported through [Babel](https://babeljs.io/) and ES module support through [webpack](https://webpack.js.org/). @@ -21,7 +21,7 @@ Working with our frontend assets requires Node (v10.13.0 or greater) and Yarn ### Browser Support -For our currently-supported browsers, see our [requirements](../../install/requirements.md#supported-web-browsers). +For supported browsers, see our [requirements](../../install/requirements.md#supported-web-browsers). Use [BrowserStack](https://www.browserstack.com/) to test with our supported browsers. Sign in to BrowserStack with the credentials saved in the **Engineering** vault of the GitLab diff --git a/doc/development/fe_guide/performance.md b/doc/development/fe_guide/performance.md index aac2258f3a3..7d249c65ebd 100644 --- a/doc/development/fe_guide/performance.md +++ b/doc/development/fe_guide/performance.md @@ -230,12 +230,12 @@ To improve the time to first render we are using lazy loading for images. This w the actual image source on the `data-src` attribute. After the HTML is rendered and JavaScript is loaded, the value of `data-src` is moved to `src` automatically if the image is in the current viewport. -- Prepare images in HTML for lazy loading by renaming the `src` attribute to `data-src` AND adding the class `lazy`. +- Prepare images in HTML for lazy loading by renaming the `src` attribute to `data-src` and adding the class `lazy`. - If you are using the Rails `image_tag` helper, all images are lazy-loaded by default unless `lazy: false` is provided. -If you are asynchronously adding content which contains lazy images then you need to call the function +When asynchronously adding content which contains lazy images, call the function `gl.lazyLoader.searchLazyImages()` which searches for lazy images and loads them if needed. -But in general it should be handled automatically through a `MutationObserver` in the lazy loading function. +In general, it should be handled automatically through a `MutationObserver` in the lazy loading function. ### Animations @@ -243,7 +243,7 @@ Only animate `opacity` & `transform` properties. Other properties (such as `top` Layout to be recalculated, which is much more expensive. For details on this, see "Styles that Affect Layout" in [High Performance Animations](https://www.html5rocks.com/en/tutorials/speed/high-performance-animations/). -If you _do_ need to change layout (for example, a sidebar that pushes main content over), prefer [FLIP](https://aerotwist.com/blog/flip-your-animations/) to change expensive +If you _do_ need to change layout (for example, a sidebar that pushes main content over), prefer [FLIP](https://aerotwist.com/blog/flip-your-animations/). FLIP allows you to change expensive properties once, and handle the actual animation with transforms. ## Reducing Asset Footprint @@ -251,7 +251,7 @@ properties once, and handle the actual animation with transforms. ### Universal code Code that is contained in `main.js` and `commons/index.js` is loaded and -run on _all_ pages. **DO NOT ADD** anything to these files unless it is truly +run on _all_ pages. **Do not add** anything to these files unless it is truly needed _everywhere_. These bundles include ubiquitous libraries like `vue`, `axios`, and `jQuery`, as well as code for the main navigation and sidebar. Where possible we should aim to remove modules from these bundles to reduce our @@ -277,9 +277,9 @@ manually generated webpack bundles. However under this new system you should not ever need to manually add an entry point to the `webpack.config.js` file. NOTE: -If you are unsure what controller and action corresponds to a given page, you -can find this out by inspecting `document.body.dataset.page` in your -browser's developer console while on any page in GitLab. +When unsure what controller and action corresponds to a page, +inspect `document.body.dataset.page` in your +browser's developer console from any page in GitLab. #### Important Considerations @@ -294,7 +294,7 @@ browser's developer console while on any page in GitLab. All GitLab JavaScript files are added with the `defer` attribute. According to the [Mozilla documentation](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-defer), this implies that "the script is meant to be executed after the document has - been parsed, but before firing `DOMContentLoaded`". Since the document is already + been parsed, but before firing `DOMContentLoaded`". Because the document is already parsed, `DOMContentLoaded` is not needed to bootstrap applications because all the DOM nodes are already at our disposal. @@ -366,9 +366,9 @@ browser's developer console while on any page in GitLab. ### Code Splitting -For any code that does not need to be run immediately upon page load, (for example, -modals, dropdowns, and other behaviors that can be lazy-loaded), you can split -your module into asynchronous chunks with dynamic import statements. These +Code that does not need to be run immediately upon page load (for example, +modals, dropdowns, and other behaviors that can be lazy-loaded) should be split +into asynchronous chunks with dynamic import statements. These imports return a Promise which is resolved after the script has loaded: ```javascript @@ -377,16 +377,16 @@ import(/* webpackChunkName: 'emoji' */ '~/emoji') .catch(/* report error */) ``` -Please try to use `webpackChunkName` when generating these dynamic imports as +Use `webpackChunkName` when generating dynamic imports as it provides a deterministic filename for the chunk which can then be cached -the browser across GitLab versions. +in the browser across GitLab versions. More information is available in [webpack's code splitting documentation](https://webpack.js.org/guides/code-splitting/#dynamic-imports). ### Minimizing page size -A smaller page size means the page loads faster (especially important on mobile -and poor connections), the page is parsed more quickly by the browser, and less +A smaller page size means the page loads faster, especially on mobile +and poor connections. The page is parsed more quickly by the browser, and less data is used for users with capped data plans. General tips: diff --git a/doc/development/fe_guide/security.md b/doc/development/fe_guide/security.md index 627c5f4d12f..df4613d521d 100644 --- a/doc/development/fe_guide/security.md +++ b/doc/development/fe_guide/security.md @@ -8,7 +8,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w ## Resources -[Mozilla’s HTTP Observatory CLI](https://github.com/mozilla/http-observatory-cli) and the +[Mozilla’s HTTP Observatory CLI](https://github.com/mozilla/http-observatory-cli) and [Qualys SSL Labs Server Test](https://www.ssllabs.com/ssltest/analyze.html) are good resources for finding potential problems and ensuring compliance with security best practices. @@ -76,7 +76,7 @@ such as with reCAPTCHA, which cannot be used without an `iframe`. In order to protect users from [XSS vulnerabilities](https://en.wikipedia.org/wiki/Cross-site_scripting), we intend to disable inline scripts in the future using Content Security Policy. -While inline scripts can be useful, they're also a security concern. If +While inline scripts can make something easier, they're also a security concern. If user-supplied content is unintentionally left un-sanitized, malicious users can inject scripts into the web app. diff --git a/doc/development/fe_guide/tooling.md b/doc/development/fe_guide/tooling.md index d33022b9355..7a2d8fccdbf 100644 --- a/doc/development/fe_guide/tooling.md +++ b/doc/development/fe_guide/tooling.md @@ -111,7 +111,7 @@ preferred editor (all major editors are supported) accordingly. We suggest setting up Prettier to run when each file is saved. For instructions about using Prettier in your preferred editor, see the [Prettier documentation](https://prettier.io/docs/en/editors.html). -Please take care that you only let Prettier format the same file types as the global Yarn script does (`.js`, `.vue`, `.graphql`, and `.scss`). In VSCode by example you can easily exclude file formats in your settings file: +Please take care that you only let Prettier format the same file types as the global Yarn script does (`.js`, `.vue`, `.graphql`, and `.scss`). For example, you can exclude file formats in your Visual Studio Code settings file: ```json "prettier.disableLanguages": [ @@ -128,13 +128,13 @@ The following yarn scripts are available to do global formatting: yarn prettier-staged-save ``` -Updates all currently staged files (based on `git diff`) with Prettier and saves the needed changes. +Updates all staged files (based on `git diff`) with Prettier and saves the needed changes. ```shell yarn prettier-staged ``` -Checks all currently staged files (based on `git diff`) with Prettier and log which files would need manual updating to the console. +Checks all staged files (based on `git diff`) with Prettier and log which files would need manual updating to the console. ```shell yarn prettier-all diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index b3fbb9556a9..79ccd966bca 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -22,7 +22,7 @@ All new features built with Vue.js must follow a [Flux architecture](https://fac The main goal we are trying to achieve is to have only one data flow and only one data entry. In order to achieve this goal we use [vuex](#vuex). -You can also read about this architecture in Vue docs about +You can also read about this architecture in Vue documentation about [state management](https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch) and about [one way data flow](https://vuejs.org/v2/guide/components.html#One-Way-Data-Flow). @@ -70,7 +70,7 @@ The advantage of providing data from the DOM to the Vue instance through `props` function instead of querying the DOM inside the main Vue component is avoiding the need to create a fixture or an HTML element in the unit test, which makes the tests easier. -See the following example, also, please refer to our [Vue style guide](style/vue.md#basic-rules) for +See the following example. Also, please refer to our [Vue style guide](style/vue.md#basic-rules) for additional information on why we explicitly declare the data being passed into the Vue app; ```javascript @@ -101,8 +101,8 @@ across the codebase. #### Accessing the `gl` object -When we need to query the `gl` object for data that doesn't change during the application's life -cycle, we should do it in the same place where we query the DOM. By following this practice, we can +We query the `gl` object for data that doesn't change during the application's life +cycle in the same place we query the DOM. By following this practice, we can avoid the need to mock the `gl` object, which makes tests easier. It should be done while initializing our Vue instance, and the data should be provided as `props` to the main component: @@ -148,8 +148,8 @@ This approach has a few benefits: - Arbitrarily deeply nested components can opt-in and access the flag without intermediate components being aware of it (c.f. passing the flag down via props). -- Good testability, since the flag can be provided to `mount`/`shallowMount` - from `vue-test-utils` simply as a prop. +- Good testability, because the flag can be provided to `mount`/`shallowMount` + from `vue-test-utils` as a prop. ```javascript import { shallowMount } from '@vue/test-utils'; @@ -207,20 +207,20 @@ Based on the Vue guidance: such as `user: new User()`. - **Do not** add new JavaScript class implementations. - **Do** use [GraphQL](../api_graphql_styleguide.md), [Vuex](vuex.md) or a set of components if -cannot use simple primitives or objects. +cannot use primitives or objects. - **Do** maintain existing implementations using such approaches. - **Do** Migrate components to a pure object model when there are substantial changes to it. -- **Do** add business logic to helpers or utils, so you can test them separately from your component. +- **Do** add business logic to helpers or utilities, so you can test them separately from your component. #### Why There are additional reasons why having a JavaScript class presents maintainability issues on a huge codebase: -- Once a class is created, it is easy to extend it in a way that can infringe Vue reactivity and best practices. +- After a class is created, it can be extended in a way that can infringe Vue reactivity and best practices. - A class adds a layer of abstraction, which makes the component API and its inner workings less clear. -- It makes it harder to test. Since the class is instantiated by the component data function, it is +- It makes it harder to test. Because the class is instantiated by the component data function, it is harder to 'manage' component and class separately. -- Adding OOP to a functional codebase adds yet another way of writing code, reducing consistency and clarity. +- Adding Object Oriented Principles (OOP) to a functional codebase adds yet another way of writing code, reducing consistency and clarity. ## Style guide @@ -234,8 +234,8 @@ for guidelines and best practices for testing your Vue components. Each Vue component has a unique output. This output is always present in the render function. -Although we can test each method of a Vue component individually, our goal must be to test the output -of the render/template function, which represents the state at all times. +Although each method of a Vue component can be tested individually, our goal is to test the output +of the render function, which represents the state at all times. Here's an example of a well structured unit test for [this Vue component](#appendix---vue-component-subject-under-test): @@ -366,7 +366,7 @@ component under test, with the `computed` property, for example). Remember to us ### Events -We should test for events emitted in response to an action within our component, this is useful to +We should test for events emitted in response to an action in our component. This is used to verify the correct events are being fired with the correct arguments. For any DOM events we should use [`trigger`](https://vue-test-utils.vuejs.org/api/wrapper/#trigger) @@ -416,7 +416,7 @@ You should only apply to be a Vue.js expert when your own merge requests and you > This section is added temporarily to support the efforts to migrate the codebase from Vue 2.x to Vue 3.x -Currently, we recommend to minimize adding certain features to the codebase to prevent increasing +We recommend to minimize adding certain features to the codebase to prevent increasing the tech debt for the eventual migration: - filters; diff --git a/doc/development/fe_guide/vue3_migration.md b/doc/development/fe_guide/vue3_migration.md index 9d2f3b27968..90bf46d9ad9 100644 --- a/doc/development/fe_guide/vue3_migration.md +++ b/doc/development/fe_guide/vue3_migration.md @@ -26,7 +26,7 @@ Component's computed properties / methods or external helpers. **What to use instead** -Vue docs recommend using [mitt](https://github.com/developit/mitt) library. It's relatively small (200 bytes gzipped) and has a simple API: +Vue documentation recommends using the [mitt](https://github.com/developit/mitt) library. It's relatively small (200 bytes gzipped) and has a clear API: ```javascript import mitt from 'mitt' @@ -51,9 +51,9 @@ emitter.off('foo', onFoo) // unlisten **Event hub factory** -To make it easier for you to migrate existing event hubs to the new recommended approach, or simply -to create new ones, we have created a factory that you can use to instantiate a new mitt-based -event hub. +We have created a factory that you can use to instantiate a new mitt-based event hub. +This makes it easier to migrate existing event hubs to the new recommended approach, or +to create new ones. ```javascript import createEventHub from '~/helpers/event_hub_factory'; @@ -88,7 +88,7 @@ It is not recommended to replace stateful components with functional components **Why?** -In Vue 2.6 `slot` attribute was already deprecated in favor of `v-slot` directive but its usage is still allowed and sometimes we prefer using them because it simplifies unit tests (with old syntax, slots are rendered on `shallowMount`). However, in Vue 3 we can't use old syntax anymore. +In Vue 2.6 `slot` attribute was already deprecated in favor of `v-slot` directive. The `slot` attribute usage is still allowed and sometimes we prefer using it because it simplifies unit tests (with old syntax, slots are rendered on `shallowMount`). However, in Vue 3 we can't use old syntax anymore. **What to use instead** diff --git a/doc/development/fe_guide/vuex.md b/doc/development/fe_guide/vuex.md index 1d83335291a..cc1d9ccab77 100644 --- a/doc/development/fe_guide/vuex.md +++ b/doc/development/fe_guide/vuex.md @@ -146,7 +146,7 @@ The only way to change state in a Vuex store is by committing a mutation. Most mutations are committed from an action using `commit`. If you don't have any asynchronous operations, you can call mutations from a component using the `mapMutations` helper. -See the Vuex docs for examples of [committing mutations from components](https://vuex.vuejs.org/guide/mutations.html#committing-mutations-in-components). +See the Vuex documentation for examples of [committing mutations from components](https://vuex.vuejs.org/guide/mutations.html#committing-mutations-in-components). #### Naming Pattern: `REQUEST` and `RECEIVE` namespaces @@ -271,7 +271,7 @@ import { mapGetters } from 'vuex'; ### `mutation_types.js` -From [vuex mutations docs](https://vuex.vuejs.org/guide/mutations.html): +From [Vuex mutations documentation](https://vuex.vuejs.org/guide/mutations.html): > It is a commonly seen pattern to use constants for mutation types in various Flux implementations. > This allows the code to take advantage of tooling like linters, and putting all constants in a > single file allows your collaborators to get an at-a-glance view of what mutations are possible @@ -429,7 +429,7 @@ export default { #### Testing Vuex concerns -Refer to [Vuex docs](https://vuex.vuejs.org/guide/testing.html) regarding testing Actions, Getters and Mutations. +Refer to [Vuex documentation](https://vuex.vuejs.org/guide/testing.html) regarding testing Actions, Getters and Mutations. #### Testing components that need a store diff --git a/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md b/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md index 42fb5f59a99..233143dc461 100644 --- a/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md +++ b/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md @@ -136,6 +136,46 @@ NOTE: You can append `?w=1` while on the diffs page of a merge request to ignore any whitespace changes. +## Mark files as viewed + +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/51513) in GitLab 13.8. +> - It's deployed behind a feature flag, enabled by default. +> - It's enabled on GitLab.com. +> - It's recommended for production use. +> - For GitLab self-managed instances, GitLab administrators can opt to [disable it](#enable-or-disable-file-views). **(CORE ONLY)** + +When reviewing a merge request with many files multiple times, it may be useful to the reviewer +to focus on new changes and ignore the files that they have already reviewed and don't want to +see anymore unless they are changed again. + +To mark a file as viewed: + +1. Go to the merge request's **Diffs** tab. +1. On the right-top of the file, locate the **Viewed** checkbox. +1. Check it to mark the file as viewed. + +Once checked, the file will remain marked for that reviewer unless there are newly introduced +changes to its content or the checkbox is unchecked. + +### Enable or disable file views **(CORE ONLY)** + +The file view feature is under development but ready for production use. +It is deployed behind a feature flag that is **enabled by default**. +[GitLab administrators with access to the GitLab Rails console](../../../administration/feature_flags.md) +can opt to enable it for your instance. + +To enable it: + +```ruby +Feature.enable(:local_file_reviews) +``` + +To disable it: + +```ruby +Feature.disable(:local_file_reviews) +``` + ## Perform inline code reviews > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/13950) in GitLab 11.5. diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index 209917073c7..53df0b7b389 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -244,6 +244,8 @@ module Gitlab def prune_diff_if_eligible if too_large? + ::Gitlab::Metrics.add_event(:patch_hard_limit_bytes_hit) + too_large! elsif collapsed? collapse! diff --git a/spec/frontend/behaviors/autosize_spec.js b/spec/frontend/behaviors/autosize_spec.js index 352bd8a0ed0..a9dbee7fd08 100644 --- a/spec/frontend/behaviors/autosize_spec.js +++ b/spec/frontend/behaviors/autosize_spec.js @@ -1,12 +1,20 @@ import '~/behaviors/autosize'; -function load() { - document.dispatchEvent(new Event('DOMContentLoaded')); -} - jest.mock('~/helpers/startup_css_helper', () => { return { - waitForCSSLoaded: jest.fn().mockImplementation((cb) => cb.apply()), + waitForCSSLoaded: jest.fn().mockImplementation((cb) => { + // This is a hack: + // autosize.js will execute and modify the DOM + // whenever waitForCSSLoaded calls its callback function. + // This setTimeout is here because everything within setTimeout will be queued + // as async code until the current call stack is executed. + // If we would not do this, the mock for waitForCSSLoaded would call its callback + // before the fixture in the beforeEach is set and the Test would fail. + // more on this here: https://johnresig.com/blog/how-javascript-timers-work/ + setTimeout(() => { + cb.apply(); + }, 0); + }), }; }); @@ -16,9 +24,15 @@ describe('Autosize behavior', () => { }); it('is applied to the textarea', () => { - load(); - - const textarea = document.querySelector('textarea'); - expect(textarea.classList).toContain('js-autosize-initialized'); + // This is the second part of the Hack: + // Because we are forcing the mock for WaitForCSSLoaded and the very end of our callstack + // to call its callback. This querySelector needs to go to the very end of our callstack + // as well, if we would not have this setTimeout Function here, the querySelector + // would run before the mockImplementation called its callBack Function + // the DOM Manipulation didn't happen yet and the test would fail. + setTimeout(() => { + const textarea = document.querySelector('textarea'); + expect(textarea.classList).toContain('js-autosize-initialized'); + }, 0); }); }); diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb index 783f0a9ccf7..17bb83d0f2f 100644 --- a/spec/lib/gitlab/git/diff_spec.rb +++ b/spec/lib/gitlab/git/diff_spec.rb @@ -100,6 +100,13 @@ EOT expect(diff.diff).to be_empty expect(diff).to be_too_large end + + it 'logs the event' do + expect(Gitlab::Metrics).to receive(:add_event) + .with(:patch_hard_limit_bytes_hit) + + diff + end end context 'using a collapsable diff that is too large' do diff --git a/spec/serializers/diffs_entity_spec.rb b/spec/serializers/diffs_entity_spec.rb index 7569493573b..a7446f14745 100644 --- a/spec/serializers/diffs_entity_spec.rb +++ b/spec/serializers/diffs_entity_spec.rb @@ -3,10 +3,11 @@ require 'spec_helper' RSpec.describe DiffsEntity do - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } + let(:request) { EntityRequest.new(project: project, current_user: user) } - let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } let(:merge_request_diffs) { merge_request.merge_request_diffs } let(:options) do { request: request, merge_request: merge_request, merge_request_diffs: merge_request_diffs } @@ -30,6 +31,14 @@ RSpec.describe DiffsEntity do ) end + context 'broken merge request' do + let(:merge_request) { create(:merge_request, :invalid, target_project: project, source_project: project) } + + it 'renders without errors' do + expect { subject }.not_to raise_error + end + end + context "when a commit_id is passed" do let(:commits) { merge_request.commits } let(:entity) do |