Page MenuHomePhabricator

Can't paste a reference after recovering from autosave
Open, HighPublic

Description

  • Open an article in VE, e.g. https://en.wikipedia.org/w/index.php?title=Daron_Acemoglu&veaction=edit
  • Add a word and a space to the article then hit reload, your edit will auto-recover
  • Paste the following plain text into the third paragraph (between references 4 & 5): <ref>Hello</ref>
  • Observe that you have a re-use of reference [3]
  • Expect that you would have a new reference (numbered [5]) with the contents you pasted.

The cause of this issue is that the DM documents persistent storage is not being saved to sessionStorage, this is because:

  • In https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/+/1050578, an unserializable object (an instance of MWDocumentReferences) was added to the persistent storage. (Note that persistent storage did not document this requirement, or that it was recovered by auto-save)
  • When the autosave code (in ve.dm.Surface) tried to serialize and store that object, it threw an error, but the SafeStorage wrapper swallowed it, so everything appeared to be working.

Event Timeline

Thanks for connecting the pieces for me! Would you agree that the fix is to strip out this.doc before serialization, or is there more to making an object serializable in VE?

I would question the need for the object to be persistent. If it can be reconstructed from the internal list it should be. If it is storing additional state I would question why that state isn't into he document model.

Good questions. +1 yes it doesn't need to be persistent, in fact it shouldn't be. The DocumentReferences are recalculated from a reference group's InternalList any time a ref from that group is edited.

Ideally, the calculated data is cached in a singleton attached to the document, purely as an optimization. It can be purged any time. The reason for this optimization at all is to work around an awkward control flow in which each ce.ReferenceNode is responsible for calculating its own footnote number.

Even better would be if the numbering were recalculated a single time after each ref-involved document change, and the calculated numbers were stored into each ReferenceNode (dm or ce) as I tried and failed to achieve in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cite/+/1057381 . This would be a much bigger architectural change as far as I can tell, though.

Change #1081407 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Document requirements for persistent storage, and enforce them

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

Change #1081604 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Cite@master] [WIP] Fix regression to auto-save

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

awight triaged this task as High priority.Oct 20 2024, 9:07 AM

Change #1081604 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Fix regression to auto-save

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

Fix did narrowly make it into REL1_43, so no backporting is required.

The issue is not solved. Pasting the ref as described causes it to show up with number "[1]", although it's not a reuse of the first reference.

Also, an error and stack trace are emitted in the console:

Uncaught TypeError: group.firstNodes[index2] is undefined
    sortGroupIndexes ve.dm.InternalList.js:422
    sortGroupIndexes ve.dm.InternalList.js:413
    onTransact ve.dm.InternalList.js:364
    emit oojs.js:858
    commit ve.dm.Document.js:419
    changeInternal ve.dm.Surface.js:971
    change ve.dm.Surface.js:940
    change ve.dm.SurfaceFragment.js:125
    insertDocument ve.dm.SurfaceFragment.js:943
    insert ve.ce.Surface.js:1751
    jQuery 3
    resolve ve.ui.DataTransferHandler.js:121
    process ve.ui.MWWikitextStringTransferHandler.js:162
    jQuery 2

There are some surprising entries in the ve-changes session storage. After making a change that involves a ref, the stored transaction will include a full copy of the internalList before and after the change, despite the difference only affecting a single item in the list.

the stored transaction will include a full copy of the internalList before and after the change

This is a known issue: T130142

The remaining issue is not caused by MWDocumentReferences. We should continue with trying to fix, but the task description is no longer describing the root cause. I found the same issue present in REL1_42 and REL1_41, before the new mechanism was introduced.

thiemowmde moved this task from Doing to Done on the WMDE-TechWish-Sprint-2024-10-16 board.

For the sake of completeness: The immediate issue seems to be fixed. What remains appears to be much older and unrelated. As such the WMDE-TechWish team is not going to pick this ticket up again as part of our current sprint work, but leave it for the Editing-team to pick up and prioritize.

  NODES
Idea 1
idea 1
INTERN 9
Note 5
Project 3