Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-899 Merge Diff improvements #902

Merged
merged 8 commits into from
Aug 29, 2024

Conversation

pjmonks
Copy link
Contributor

@pjmonks pjmonks commented Aug 10, 2024

Resolves #899

Overview

Updated the Merge Diff UI with a number of improvements to make the user experience better with large Versioned Folders (NHS Data Dictionary branches)

  • Massively improved the performance of the diff lists. When very large, Angular would spend ages rendering each row. Now uses cdk-virtual-scroll-viewport on all diff lists to only render what is visible, makes a big speed and responsiveness improvement.
  • Fix styling of rows to not squash text
  • Add progress bar indicators when long running work is carried out - calculating diffs, committing etc
  • Fix styling of panels so the change lists and the selected comparison are more visible on the page together

image

Testing

Test on an in-flight NHS Data Dictionary branch and merge into main. Calculating the diffs will take time (around 5 minutes), but then the UI should be responsive after that without slowdown.

- Don't squash rows by forcing height
- Add border to separate rows clearer
- Use cdk-virtual-scroll-viewport to massively speed up rendering large lists of diff items
- Update accept/commit logic to force new immutable arrays to to be assigned, which will force the virtual scoll viewports to render again on updates
- Show path name in merge comparison component more prominently
- Adjust positions and sizes of components
- Use cdk-virtual-scroll-viewport on large lists of diffs to display
@OButlerOcc OButlerOcc self-assigned this Aug 14, 2024
Copy link
Contributor

@OButlerOcc OButlerOcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had problems with this one:

NHSD: Develop
MDM-UI: gh-899
Mdm-Core: James's build
MDM-App build: develop
Mdm-resources: Develop

I Think its rendering it in the wrong box:
image

I think reingesting got it?
image

Question, why do I sometimes have commits but no changes? I assume those are my local commits but please confirm so I know its not a bug.

Issue 2:
500 Server error occurred while waiting for merge diff. I'm going to try reingesting all my branches.

2024-08-14 22:31:50,253 [080-exec-1] ERROR o.g.web.errors.GrailsExceptionResolver   : LazyInitializationException occurred when processing request: [GET] /api/versionedFolders/c49ec084-3b51-4f98-86ee-f3d75b05699c/mergeDiff/e1baf1a6-b8a0-4ccf-80d6-377fa86756ec - parameters:
isLegacy: false
failed to lazily initialize a collection of role: uk.ac.ox.softeng.maurodatamapper.core.facet.Annotation.childAnnotations, could not initialize proxy - no Session. Stacktrace follows:

Issue 3:
Can falsely try and merge finalised branches, causes crash on diff

image

Unmerged backend branch?

@pjmonks
Copy link
Contributor Author

pjmonks commented Aug 28, 2024

@OButlerOcc To answer your questions:

  1. I've tried to render the row height to fit everything inside, but there are some paths which are just too long regardless and outside of our control. This is the best I think we can do for now, I would suggest the pragmatic way forward is to just accept this and fix rendering issues NHSE raise later.
  2. The "Committing" and "Changes" lists are automatically loaded after the initial comparison, then manually altered by the user if they wish. After the initial comparison, Mauro may have decided that it is possible to automatically commit some changes because it is clear they have no conflicts - this is why the "Committing" list may have everything and the "Changes" list has nothing. Ultimately all the changes are part of the diff, the "Committing" list is just what will definitely be merged when you click "Commit Changes".
  3. Any errors from the backend will have to be resolved in mdm-core, this is just the UI changes.
  4. I am not able to select a finalised branch to merge either as a source or a target. Do you have the latest mdm-core and mdm-ui changes? I'm sure you or I fixed the target list to never show finalised branches.

We should discuss further issues you have in person.

Copy link
Contributor

@OButlerOcc OButlerOcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'm happy with this one. I think My previous concerns were caused by the build mess and having some dependencies/plugins cause incompatibility.

I've been through the code and as its visible cosmetic and markup html changes there's no problems with clarity I can see as theres a big

tags explaining what's going on.

TESTED:

  • Merging dif took 6 min for branch 1903 and 5 min for 1883
  • Row styling is fine, gets a bit squashed sometimes with tiny windows but wouldn't be doing things like this on a phone anyway
  • Progress and working indicators are present and help a lot with figuring out what its doing
  • responsiveness is there once its finished calculating. Stopping the calc restores function fine.

Issues.
Stuff in the uncommited changes box gets warped when merging, not worth fixing but its there, possible future improvement

image

Importing an inflight branch without an import date causes a weird bug where the branch has an empty label or name field. When merging it tries to create a model with no name and has a invalid model error. This is a backend error and thus not part of this task. but It is a crash

there are issues merging the old in flight branches with the new model as it contains duplicate data, this is an issue with the test data, not the system (I think)

@joe-crawford joe-crawford merged commit 46e9dbb into develop Aug 29, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test that NHS Data Dictionary in-flight branches can be merged
3 participants