In the old Web IDE, opening from an MR also opened all changed files. This was helpful for those looking to review the MR changes in the Web IDE or add additional changes to modified files.
Proposal
We should bring back this feature to the new Web IDE.
Opening the Web IDE from an MR should open the modified files in separate tabs.
Details
For performance reasons, we should limit this to opening the first 10 files modified in the MR.
This issue's description does not seem to have a section for "Implementation Guide".
Please consider adding one, because it makes a big difference for contributors.
This section can be brief but must have clear technical guidance, like:
Hints on lines of code which may need changing
Hints on similar code/patterns that can be leveraged
Suggestions for test coverage
Ideas for breaking up the merge requests into iterative chunks
Great question. As the extension is still a few milestones out, do you think this would be a good solution until then?
Also, once we have the extension ready, do you think having all changed files visible in the tab in the left sidebar is good enough, or should they also be pre-opened?
I'm afraid of many tabs if it's a huge MR. Navigation with tabs in the tab bar is not as easy, but a bit easier with tabs in the left pane (if there are more than 5 files)
The average MR has 2 changed files, but it jumps to ~15 files on the 90%ile and ~124 on the 99%ile.
FWIW, it's worth noting the obvious that not all MR's are the same.
The more important an MR, the more developers would like their tools to work nicely. So although there are technically more 1-2 files changed MR's in the wild, it's arguable that the importance of the MR increases when there are more files changed.
As someone who regularly reviews and creates MR's, the 10-15 files changed case is a very regular occurrence which I'd like us to be able to handle gracefully.
Myself and @pslaughter spoke yesterday about this and wanted to see what y'all thought about adding a group to the SC plane which would represent the diff from an opened MR? This would sit under the group that is created for "current changes".
We might even consider a slightly different badge experience on the SC icon in the left sidebar to note files that are previously changed from a diff as that would given context to a user on where to view those changes.
@pslaughter@oregand I think the larger idea makes sense, but I'm not sure about the execution, as at least I would have gotten confused how these merge request changes are connected to the changes in the commit I'm going to make.
Taking your general approach but slightly changing the execution, what do you all think about maybe taking over the "Open Editors" approach, and adding "Merge Request Changes" as expandable section at the top of the "Explorer" tab?
If that's the way we want to go, we would need to think about cases where there are so many changed files that this expandable section is the only one visible, would it maybe be possible to add a max-height to it to ensure that the "All files" section would always show up?
I would have gotten confused how these merge request changes are connected to the changes in the commit I'm going to make.
This is my biggest concern as well but I'm not sure how we resolve it. I think the base expectation coming from the "old" Web IDE is exactly what @pslaughter showed, but we also didn't have a "staging" area to deal with.
@mvanremmerden Maybe you remember, how did we handle this when we did allow staging files in the Web IDE?
I think we always opened all of the changed files by default, and didn't list or highlight them anywhere else.
This is correct @mvanremmerden, there's also the very odd "Review" tab which duplicates the "File Explorer" but now decorates files when they are part of the MR (notice that there's no way to view what files are removed in the MR)
Do you happen to have any MRs that roughly fit the criteria from 99th percentile (~125 changed files, as shared by Pedro above #386256 (comment 1250490799)), that we could use to make sure we know if there are any performance hits we would take?
This does mean that there will be a significant delay until all the files show up (like there is in the old Web IDE).
IMHO, opening all modified files might work for some MR's, but in general it was always a pretty confusing behavior of the old Web IDE
Do you happen to have any MRs that roughly fit the criteria from 99th percentile (~125 changed files, as shared by Pedro above #386256 (comment 1250490799))
I don't have any MR's like this off-hand, but it's not great for the ~10 changed files use case
I agree we can iterate forward with this and look to benchmark feedback from our users. We would need to establish how many MRs with 20+ file changes are regularly opened from the Web IDE before we consider looking at a different solution.
One thing that might be worth thinking about is how this experience works with the GitLab VS Code Extension if that becomes enabled by default in the Web IDE. That has it's own view for MRs where you can review, but you can't edit code... so there might be different experiences depending on context.
Great point @phikai, this is going to be a reality sooner or later. @jmiocene let's take a look at the experience in the extension and make sure it doesn't conflict with whatever iteration we build for this issue. Or, if necessary, table this issue until we have the extension integrated?
Or, if necessary, table this issue until we have the extension integrated?
@ericschurter I think it's best to solve this issue before integrating the extension. Integration the extension is a very weighty undertaking in comparison to presenting the diff of the MR. We can easily revert this iterative "view MR" experience once we have integration with the VSCode extension.
@pslaughter Sounds great, I agree. Of the feedback I hear about the Beta, the change in behavior here has been the most disruptive (at least within GitLab). I'm not saying the previous experience was perfect, but we were used to it
What if we had an area at the bottom of the SCM panel that had a Commit button if changes have been made and a "View Changed Files" if there's an existing MR? Lots of edge cases to think through here, but we can work with @jmiocene to figure it out
This ~"group::editor" bug has at most 50% of the SLO duration remaining and is ~"approaching-SLO" breach. Please consider taking action before this becomes a ~"missed-SLO" in 24 days (2023-03-07).
Not to undermine the severity or importance of this issue, but as we're getting pinged on SLO it might be worth considering whether this is actually a bug. It's behaving as intended, and while it's arguably a step back in usability when compared to the previous Web IDE, as we've discussed in the comments here, it may have an entirely different resolution. I'd categorize this as a typefeature myself. WDYT @oregand@mvanremmerden ?
IMHO that's a missing feature compared to the old Web IDE, and should be considered for when the new Web IDE becomes GA. Which I think will happen if I interpret the milestone %15.10 correctly?
As a customer who used to use this feature, I have now given up and use non-gitlab tools to manage merge reviews. It's disappointing to lose this functionality, but hey, prioritization is prioritization. Can't make everyone happy!
@bluntjackson We hear you, this is definitely a feature we're going to bring over, we're just looking for the right solution. This workflow is still available in the non-Beta Web IDE, so you can continue to use that until we deliver this feature.
@pslaughter, @jmiocene This issue looks like it may slip this current milestone. Can you leave a or to signify if you are on track to deliver this issue?
Please also consider updating the issue's Health Status or Milestone to reflect its current state,
and communicate with your Product Manager as appropriate.