Resolve "When the branch name is a divider or separator, the project home page cannot display the branch name correctly."
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 divider
s and separator
s 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 separatorli
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 |
---|---|
![]() |
![]() |
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry for user-facing changes, or community contribution. Check the link for other scenarios. -
Documentation created/updated or follow-up review issue created -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Performance and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers
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)
Merge request reports
Activity
changed milestone to %Backlog
added devopscreate frontend groupsource code priority4 repository severity4 typebug labels
added 1 commit
- edfac052 - Use a more obscure string for dropdown separators
added 1 commit
- 2a7b2f45 - Use a more obscure string for dropdown separators
2 Warnings This merge request changed files with disabled eslint rules. Please consider fixing them. The title of this merge request is longer than 72 characters and would violate our commit message rules when using the Squash on Merge feature. Please consider adjusting the title, or rebase the commits manually and don’t use Squash on Merge. Disabled eslint rules
The following files have disabled
eslint
rules. Please consider fixing them:app/assets/javascripts/gl_dropdown.js
app/assets/javascripts/labels_select.js
app/assets/javascripts/milestone_select.js
app/assets/javascripts/namespace_select.js
app/assets/javascripts/search_autocomplete.js
app/assets/javascripts/users_select.js
spec/javascripts/gl_dropdown_spec.js
Run the following command for more details
node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \ 'app/assets/javascripts/gl_dropdown.js' \ 'app/assets/javascripts/labels_select.js' \ 'app/assets/javascripts/milestone_select.js' \ 'app/assets/javascripts/namespace_select.js' \ 'app/assets/javascripts/search_autocomplete.js' \ 'app/assets/javascripts/users_select.js' \ 'spec/javascripts/gl_dropdown_spec.js'
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.
Category Reviewer Maintainer frontend Illya Klymov ( @xanf
)Paul Slaughter ( @pslaughter
)Generated by
DangerEdited by 🤖 GitLab Bot 🤖marked the checklist item Changelog entry for user-facing changes, or community contribution. Check the link for other scenarios. as completed
marked the checklist item Code review guidelines as completed
marked the checklist item Merge request performance guidelines as completed
added 1 commit
- 20b1905b - Use a more obscure string for dropdown separators
marked the checklist item Style guides as completed
assigned to @thomasrandolph
added 1 commit
- 32118ff9 - Use a more obscure string for dropdown separators
added 1 commit
- caf0b0ab - Use a more obscure string for dropdown separators
added 1 commit
- 643a696a - Use a more obscure string for dropdown separators
added backstage [DEPRECATED] label
added 51 commits
-
643a696a...e2251a09 - 50 commits from branch
master
- 1043de5c - Use a more obscure string for dropdown separators
-
643a696a...e2251a09 - 50 commits from branch
mentioned in issue thomasrandolph/todo#1 (closed)
marked the checklist item Tested in all supported browsers as completed
added 1 commit
- 2fd508e7 - Use a more obscure string for dropdown separators
added 5 commits
-
2fd508e7...78db8d82 - 4 commits from branch
master
- f9aa6a9b - Use a more obscure string for dropdown separators
-
2fd508e7...78db8d82 - 4 commits from branch
added 8 commits
-
f9aa6a9b...840540e3 - 7 commits from branch
master
- acec4317 - Use a more obscure string for dropdown separators
-
f9aa6a9b...840540e3 - 7 commits from branch
assigned to @mfluharty and unassigned @thomasrandolph
Hi, @mfluharty! Assigning to you for review!
@mfluharty I'm going to pull this back to dev to try a better solution.
assigned to @thomasrandolph and unassigned @mfluharty
added 132 commits
-
acec4317...7848a88a - 128 commits from branch
master
- 19005459 - Extract and refactor dropdown item rendering
- 717f0d93 - Update all uses of gl_dropdown to use objects
- 41203818 - Remove unused variable
- 71cc7c67 - Add changelog for fixing the ref switcher
Toggle commit list-
acec4317...7848a88a - 128 commits from branch
assigned to @xanf and @andr3 and unassigned @thomasrandolph
- Resolved by Thomas Randolph
@andr3 & @xanf: Assigning to you for review. This is a much larger change than I had initially anticipated with this issue, but I think it's the right combination of complete (the issue can't happen any more) and boy-scouted (the code is drastically better, in my opinion), and quick (it only took a few hours to do).
Definitely looking for any feedback since this is the most significant change I've made yet.
Happy to describe the changes in person if the MR / Commits don't make the changes clear enough!
Thanks!
added 21 commits
-
7394961c...5c8545ed - 16 commits from branch
master
- 0289a63f - Extract and refactor dropdown item rendering
- a23b3511 - Update all uses of gl_dropdown to use objects
- 2e67d778 - Remove unused variable
- c7fad315 - Add changelog for fixing the ref switcher
- a4435e01 - Update explicit headers to new format
Toggle commit list-
7394961c...5c8545ed - 16 commits from branch
added 1 commit
- a5621b5f - Don't get the parent if the dropdown doesn't exist
added 11 commits
-
a5621b5f...96f2b646 - 4 commits from branch
master
- a776b745 - Extract and refactor dropdown item rendering
- 55ccefa6 - Update all uses of gl_dropdown to use objects
- 740ac2a1 - Remove unused variable
- b39260c3 - Add changelog for fixing the ref switcher
- 839beaf3 - Update explicit headers to new format
- 08ffe386 - Don't get the parent if the dropdown doesn't exist
- 5c10627f - Default selected to the opposite of the chunk id
Toggle commit list-
a5621b5f...96f2b646 - 4 commits from branch
added 51 commits
-
5c10627f...1c4ca89a - 47 commits from branch
master
- 23ad976b - Extract and refactor dropdown item rendering
- c2385329 - Update all uses of gl_dropdown to use objects
- d1f96375 - Remove unused variable
- ad7ed9be - Add changelog for fixing the ref switcher
Toggle commit list-
5c10627f...1c4ca89a - 47 commits from branch
- Resolved by Thomas Randolph
next step: IMO it would be good to create issue in
gitlab-ui
to keep feature parity between our new dropdown logic andGlDropdown
since in future we should move togitlab-ui
components. WDYT, @andr3?
- Resolved by Thomas Randolph
Thank you for great MR. I really love your solid approach to this task and power of your code.
Let me oppose your decision a bit:
- All of the functions are idempotent (or at least intended to be)
Unfortunately it's extremely hard to speak about idempotency when we are dealing with DOM. Cloning items back-and-forth is not just performance concern, but just not a way DOM works. This leads us to different approaches mixed in your MR. When we need to add content to DOM node we're doing something like
link.innerHTML = text;
just because there is no other way for us to do that. But in your code for
hideRow
instead of directly assigningli.style.display
tonone
we're cloning node before. While I totally support immutable structures as much as possible, my primary concern is that we have two different approaches dealing with HTML:- immutable one (like
hideRow
) - mutable one (sigh, DOM)
@thomasrandolph WDYT of sticking to mutable approach for the sake of simplicity?
I've also left some comments here & there, but my main concern is about complex code around DOM mutations
- Resolved by Thomas Randolph
- Resolved by Thomas Randolph
- Resolved by Thomas Randolph
- Resolved by Thomas Randolph
- Resolved by Thomas Randolph
- Resolved by Illya Klymov
- Resolved by Illya Klymov
- Resolved by Thomas Randolph
- Resolved by Thomas Randolph
@thomasrandolph You've done some nice refactoring here!
I generally agree with what Illya has pointed out so far, I would further emphasize:
- We definitely need to add test coverage for this specific case (a dropdown entry named "divider" or "separator") to
gl_dropdown_spec.js
, and maybe review that spec while this is still fresh in your mind to make sure other dropdown options are covered properly - Some function names are not quite aligned with what the functions do and could cause confusion in future troubleshooting (
hideRow()
,handleOptions()
, etc.)
... and I've opened one additional question/discussion
- We definitely need to add test coverage for this specific case (a dropdown entry named "divider" or "separator") to
assigned to @thomasrandolph
unassigned @xanf
added 93 commits
-
ad7ed9be...92c15ec2 - 89 commits from branch
master
- eaa173e6 - Extract and refactor dropdown item rendering
- 195178af - Update all uses of gl_dropdown to use objects
- aed72c1d - Remove unused variable
- 1a0779a1 - Add changelog for fixing the ref switcher
Toggle commit list-
ad7ed9be...92c15ec2 - 89 commits from branch
unassigned @andr3
added 33 commits
Toggle commit listadded 60 commits
Toggle commit listadded 7 commits
Toggle commit listadded 7 commits
Toggle commit list@xanf & @mfluharty Thank you for the great reviews!
I've made some significant changes to the MR.
Some of the notable things:
- I moved all of the code that resolves the options + chunk properties to some value into a single map
- A couple of them (
getURL
andgetText
) could share a generic function - Regardless of code re-use, I think this better groups things that resolve "something" from the chunk and "something" from the options into a single value
- A couple of them (
- I added tests
- Covers all special cases:
separator
,divider
, andheader
- Also covers the new logic that short-circuits everything else if the item is hidden
- Covers all special cases:
I think the changes address all of your other comments, but please let me know if anything is still amiss!
- I moved all of the code that resolves the options + chunk properties to some value into a single map
assigned to @xanf and @mfluharty and unassigned @thomasrandolph
added 13 commits
Toggle commit listadded 77 commits
Toggle commit listadded 9 commits
Toggle commit list- Resolved by Illya Klymov
- Resolved by Thomas Randolph
- Resolved by Illya Klymov
@thomasrandolph Just a few more concerns, looks great!
assigned to @thomasrandolph and unassigned @xanf
added 216 commits
Toggle commit listassigned to @xanf and unassigned @mfluharty and @thomasrandolph
- Resolved by Thomas Randolph
@xanf I've updated to remove the ESLint overrides (and carve out exceptions to the property re-assignment for just the three variables that need it). Sending back to you for the last sign-off!
added 10 commits
Toggle commit listassigned to @thomasrandolph
assigned to @pslaughter and unassigned @xanf and @thomasrandolph
@pslaughter Looking for a review / merge!
- Resolved by Thomas Randolph
issue: Looks like there's a few uses of the old
'divider'
in the EE repository we need to update as well:ee/app/assets/javascripts/pages/admin/application_settings/ci_cd/ci_template.js:36
ee/app/assets/javascripts/projects/settings/access_dropdown.js:377
ee/app/assets/javascripts/projects/settings/access_dropdown.js:388
To do this, we'll need to create an EE port of this MR that contains these changes + EE specific ones. Feel free to ping me if you have any questions about this process
added 117 commits
Toggle commit listissue: So... Your MR and I went on a little adventure together
Wait! Why?
In case you haven't noticed,
gl_dropdown
is used in a lot of places...git grep -e 'glDropdown' | wc -l 75
Also, as you're well aware,
gl_dropdown
isn't pretty (I think it's a relic from the ol' coffeescript days), so changes to it have a high probability of introducing bugs. (By the way, I love how you isolated your change to this one wild function!)Long story short, if we want to make a refactor like this, we need to be very very sure it doesn't introduce bugs (oh yeah... and modules like this don't have the best testing coverage either)...
Okay Paul, so how do we make sure it doesn't introduce bugs when it's largely untested?
With a Pinning Test!
(shoutout to a very insightful and frightening book called Working Effectively with Legacy Code)
So lets create a pin of
GitLabDropdown.renderData
using Jest snapshots. In the following commit, I created a snapshot for each significant permutation of input and state after analyzingrenderItem
andrenderData
:After applying this MR ontop of that branch and updating the spec to match the new API, there were a few changes in the snapshots:
- The order of
is-active
is different
- The original renders the element even if hiding
- The original allowed passing gl_dropdown instance to renderRow (most significant)
Here's a patch to fix these issues:
0001-Fix-gl_dropdown-render-failing-snapshots.patch
Could you please review and apply this patch? This way we can assuredly say that this MR is a pure refactor except for the API bug fix. Thanks!
- The order of
Thank you very much for this patch!
In my estimation, it's A+ and I applied it.
That said:
- A previous review suggested short-circuiting the render if the item was hidden, and I agreed - it seems wasteful
- A codebase search for the
instance
variable being passed to the arbitraryrenderRow
function returned only one result... and the variable was not used in the body of that function.
Between these items and the ordering of the CSS classes, I have a question: how frequently might a change like the order of CSS classes or removing an unused function parameter cause unintended side-effects? Or - put another way - how much consternation should I have in the future about seemingly "trivial" changes like the order of a class list?
P.S. I can totally imagine how some other JS might expect hidden items to be rendered, so that one makes sense to me, but it bums me out :)
A previous review suggested short-circuiting the render if the item was hidden, and I agreed - it seems wasteful
I agree. It's wasteful :/
Can we possible address this in a follow up MR?
A codebase search for the
instance
variable being passed to the arbitraryrenderRow
function returned only one result... and the variable was not used in the body of that function.Good investigation!
how frequently might a change like the order of CSS classes or removing an unused function parameter cause unintended side-effects?
Probably never, but stranger things have happened
Or - put another way - how much consternation should I have in the future about seemingly "trivial" changes like the order of a class list?
This is a good question. The reason I'm being hyper detailed about this is that these old classes are notoriously fragile and used all over the place. It's risky to add behavior, or refactor. It's even more risky to add behavior and refactor. We don't want bug fix MR's to be reverted because a refactoring introduced another issue.
All of this to say, I prefer to be very explicit that a refactor is a pure refactor with no unintended side-effects. I think the pinning testing technique gives us confidence in this. If the pinning test fails, I get nervous
Sometimes this isn't always possible to refactor without introducing trivial side-effects, but in this case I think it is avoidable and preferred.
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
Thanks @thomasrandolph for braving to refactor and simplify some of this legacy code! Changes to these kinds of site-wide modules make me nervous, and even after all the checking, there's a risk that this introduces some bugs. On the other hand, there's a huge benefit with us untangling some of this so that it will be easier to translate to Vue in the future
I left some comments and patches. I'd recommend checking out these two first:
- https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32198#note_212736225
- https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32198#note_212814941
Back to you!
assigned to @thomasrandolph and unassigned @pslaughter
added 66 commits
Toggle commit listadded 7 commits
Toggle commit list@pslaughter Back to you! I've applied all your recommendations and patches, and left a few comments. Only one of which actually raises a question, but the others may spark some conversation :)
I've also assigned the corollary EE MR to you, since it's a dependency.
assigned to @pslaughter and unassigned @thomasrandolph
update: @thomasrandolph, thanks a lot for working on this! I'm excited about the refactor and the fix and I appreciate you letting me be nitpicky about keeping the refactor as pure as possible.
I'm 80% certain this MR is good to go. I'd like to test it locally once more, but I won't be able to do that until tomorrow. Stay tuned!
added 121 commits
Toggle commit listHey!
GitLab is moving all development for both GitLab Community Edition and Enterprise Edition into a single codebase. The current
gitlab-ce
repository will become a read-only mirror, without any proprietary code. All development is moved to the currentgitlab-ee
repository, which we will rename to justgitlab
in the coming weeks.As part of this migration, issues will be moved to the current
gitlab-ee
project. Merge requests can not be moved, and will be closed and locked instead.On September 22, 2019 we will start closing all remaining CE merge requests. We have also disabled merging of merge requests in CE.
Here is what you should do to make sure your changes are merged before then:
- Continue the development process as usual.
- When the merge request has been reviewed, create a new merge request with these changes at https://gitlab.com/gitlab-org/gitlab-ee/merge_requests.
- In this new merge request, mention the old merge request.
- Close the old merge request, and in a comment mention the new merge request.
- Assign the merge request to the person who last reviewed it, or @ mention them if you are not able to assign the merge request to them.
When creating your EE merge request, you can follow these steps to port over your changes:
- Add your local copy of GitLab CE as a Git remote to your copy of
GitLab EE. This is done using
git remote add local-ce path/to/ce/.git
. - Run
git fetch local-ce BRANCH
, whereBRANCH
is the branch containing your CE changes. - For every commit in your CE MR, run
git cherry-pick COMMIT
, with COMMIT being the commit SHA. Squashing your CE commits before may make this process easier. - Push these commits to a branch of fork like you normally would when submitting your changes.
- Submit these changes as a merge request at https://gitlab.com/gitlab-org/gitlab-ee/merge_requests.
More information about this can be found here: https://docs.gitlab.com/ee/development/automatic_ce_ee_merge.html#cherry-picking-from-ce-to-ee
If you have any questions about all of this, please ask them in our dedicated FAQ issue:
https://gitlab.com/gitlab-org/gitlab-ee/issues/13855
You can also find more information here:
https://gitlab.com/gitlab-org/gitlab-ee/issues/13304
- https://about.gitlab.com/2019/08/23/a-single-codebase-for-gitlab-community-and-enterprise-edition/
I'm going to close this MR in favor of the EE port (which refers back to this one for the writeup/discussion): https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/16165
assigned to @thomasrandolph and unassigned @pslaughter
mentioned in merge request gitlab!16165 (merged)
mentioned in merge request gitlab!28330 (merged)