Skip to content
Snippets Groups Projects

Resolve "When the branch name is a divider or separator, the project home page cannot display the branch name correctly."

Closed Resolve "When the branch name is a divider or separator, the project home page cannot display the branch name correctly."
65473-when-the-branch-name-is-a-divider-or-separator-the-project-home-page-cannot-display-the-branch-name-correctly into master
Closed Thomas Randolph requested to merge 65473-when-the-branch-name-is-a-divider-or-separator-the-project-home-page-cannot-display-the-branch-name-correctly into master

What does this MR do?

This MR fixes an issue where branches named separator or divider don't show up in the project ref switcher, but do cause it to render an actual divider or separator dropdown item.

Problem

The gl_dropdown.js file reads in string primitives for each data item. If these strings match either divider or separator, the file doesn't render the data, and instead renders an actual divider or separator item.

Unfortunately, "separator" and "divider" are much too easy to accidentally conflict with.

Solution

This MR completely refactors the rendering portion(s) of the gl_dropdown.js file.

All dividers and separators must be generated by { "type": ("separator"|"divider") } instead of plaintext strings.

More solution notes from commit c91f915a:

Basic Intent: Allow all branch names without accidentally creating layout or backstage DOM. e.g. a branch named separator should never create a separator li element.

Ideally, there should never be a string that could cause this kind of conflict.

Implementation: All of GitLabDropdown.renderItem is extracted to a standalone module.

To render a divider or separator, consumers must now pass in an object like { "type": "divider" } or { "type": "separator" }

Notable choices:

  • All of the functions have a cyclomatic complexity of 3 or less
    • See: https://en.wikipedia.org/wiki/Cyclomatic_complexity
    • Note the "Correlation to number of defects" section
    • While software complexity may not have a directly causal relationship with defects, less complex software is generally easier to reason about, and may reduce defects. I personally try to maintain complexity of no higher than 3.

Other Notes

There's an epic issue that addresses the double scrollbar problem that occurs when the branch/tag/commit list is scrollable, which is visible in the after screenshot (since the list is now long enough to scroll, barely), or on gitlab.com.

Screenshots

Before After
sep-before sep-after

Does this MR meet the acceptance criteria?

Conformity

Performance and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • ~~Label as security and @ mention @gitlab-com/gl-security/appsec~~
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Closes #65473 (moved)

Edited by Thomas Randolph

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
Please register or sign in to reply