Structured diff UI assumes all members are scalars or arrays
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

Authored By

	bd808
	Feb 7 2022, 10:47 PM

Description

The business logic coded into vue/src/views/ToolRevisionsDiff.vue and vue/src/views/ListRevisionsDiff.vue to convert a jsondiff record into a visualization of the old & new state of the record assumes that all paths within a diff will be either leaf nodes or indexes into linear arrays.

For toolinfo data this assumption will no longer be true with the implementation of T293565: Allow multiple authors in toolinfo.json and/or T299557: [Tech spike] Experiment with storage implementations for toolinfo annotations. The logic needs to be updated to properly handle nested objects and collections of objects. It seems reasonable that things should also be refactored so that toolinfo and list diff display can use the same core logic via composition rather than copy and paste.

Details

Related Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	toolhub: Bump container version to 2022-03-15-002555-production	operations/deployment-charts	master	+1 -1
	ui: Refactor and update diff rendering behavior	wikimedia/toolhub	main	+910 -451

Customize query in gerrit

Related Objects

Mentioned In: T301897: Unexpected "isTrusted" value showing up in some author diff output
T300513: Unexpected list diff api endpoint behavior when diffing lists of tools
Mentioned Here: T300513: Unexpected list diff api endpoint behavior when diffing lists of tools
T293565: Allow multiple authors in toolinfo.json
T299557: [Tech spike] Experiment with storage implementations for toolinfo annotations

Event Timeline

bd808 created this task.Feb 7 2022, 10:47 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 7 2022, 10:47 PM

T300513: Unexpected list diff api endpoint behavior when diffing lists of tools is a related but separate issue with the existing rendering system and it's assumptions. That one is more of a failed assumption in how jsondiff represents removing/editing consecutive values in a linear array.

bd808 changed the task status from Open to In Progress.Feb 9 2022, 6:17 PM

bd808 claimed this task.

bd808 triaged this task as Medium priority.

bd808 moved this task from Backlog to In Progress on the Toolhub board.

Restricted Application added a project: User-bd808. · View Herald TranscriptFeb 9 2022, 6:17 PM

bd808 mentioned this in T300513: Unexpected list diff api endpoint behavior when diffing lists of tools.Feb 11 2022, 5:54 PM

Change 762476 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[wikimedia/toolhub@main] ui: Refactor and update diff rendering behavior

https://gerrit.wikimedia.org/r/762476

gerritbot added a project: Patch-For-Review.Feb 14 2022, 4:27 PM

bd808 changed the task status from In Progress to Open.Feb 16 2022, 12:08 AM

bd808 moved this task from In Progress to Review on the Toolhub board.

Known issues with my current patch:

Backend response can return an 'add' of an array or object (or even array of objects) as a single patch instruction. This then renders a json blob in the visual diff. We would like to add logic to break that down into a row per "leaf" change. By that I mean that each row should show a single scalar value being changed.
Long values without whitespace can trigger horizontal scrolling (for example adding a long URL). We should hint to the browser with css rules that it can be more aggressive about wrapping these values.

bd808 moved this task from Review to In Progress on the Toolhub board.Feb 16 2022, 5:31 PM

bd808 mentioned this in T301897: Unexpected "isTrusted" value showing up in some author diff output.Feb 16 2022, 5:41 PM

In T301206#7715470, @bd808 wrote:

Known issues with my current patch:

🤞 These should be fixed now.

Change 762476 merged by jenkins-bot:

[wikimedia/toolhub@main] ui: Refactor and update diff rendering behavior

https://gerrit.wikimedia.org/r/762476

Maintenance_bot removed a project: Patch-For-Review.Feb 23 2022, 10:10 AM

bd808 closed this task as Resolved.Feb 23 2022, 4:30 PM

Change 770638 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[operations/deployment-charts@master] toolhub: Bump container version to 2022-03-15-002555-production

https://gerrit.wikimedia.org/r/770638

gerritbot added a project: Patch-For-Review.Mar 15 2022, 12:34 AM

Change 770638 merged by jenkins-bot: