Mobile (testing environment): Last editor input is lost if changes are made less than about 100ms before closing the app

personalizedrefrigerator commented 1 month ago

Operating system

Linux

Joplin version

86c035c90973e7b9824d7fefb3134bcaa94ad563

Desktop version info

N/A

Current behaviour

Updating the mobile note editor, then quickly unmounting the component results in loss of the last-entered data.

In particular,

In a testing environment, waiting > 100ms since the last edit seems to consistently allow the last changes to be saved.
If the component is unmounted < 100ms (e.g. 98ms) since the last edit, the last changes are lost.

On a real device, this 100ms threshold may be greater.

Test case: commit.

Expected behaviour

The last change to the editor should be preserved, even if made very briefly before closing the editor.

Notes

Similar to https://github.com/laurent22/joplin/issues/11124, but probably a different issue (#11124 is specific to sync).

Logs

No response

mrjo118 commented 2 days ago

Does unmounting the component happen just when exiting the app, or does it happen also when exiting edit mode to save the note?

mrjo118 commented 1 day ago

I can see that if I add a log entry in the componentWillUnmount() function in app-mobile Note.tsx, that it gets called when exiting the note completely i.e. going back to the note list. It isn't getting called if I close the app via the Android recent list while the note is open.

Should the title of this issue therefore instead be: 'Last editor input is lost if changes are made less than about 100ms before closing the note' ?

Also, maybe the problem and solution outlined here could aid with a resolution for the issue? https://medium.com/@minduladilthushan/resolving-the-state-not-updating-immediately-issue-in-react-js-febf5959c0cf

personalizedrefrigerator commented 1 day ago

Should the title of this issue therefore instead be: 'Last editor input is lost if changes are made less than about 100ms before closing the note' ?

Possibly, though it depends on the cause of the issue:

Note.tsx uses an AsyncActionQueue to schedule note saves. If scheduled items in the queue aren't being processed when the app is closed from the recent app list, then the title should remain "before closing the app".
- The relevant AsyncActionQueue is created here: https://github.com/laurent22/joplin/blob/0e1f6f48ef846f85430b29259dda8809218ef968/packages/app-mobile/components/screens/Note.tsx#L614
The note editor runs in a WebView, which transfers its updated content to Note.tsx when changed. If this issue is caused when the last editor content fails to transfer to Note.tsx, then the issue title should be changed to end with "before closing the note".

mrjo118 commented 1 day ago

I had a play and I was able to make your test in https://github.com/laurent22/joplin/pull/11126/files pass with a 50ms wait for jest.advanceTimersByTimeAsync.

It looks like this code in Note.tsx is the cause: // Avoid saving immediately -- the NoteEditor's content isn't controlled by its props // and updating this.state.note immediately causes slow rerenders. // // See https://github.com/laurent22/joplin/issues/10130 private onMarkdownEditorTextChange = debounce((event: EditorChangeEvent) => { shared.noteComponent_change(this, 'body', event.value); }, 100);

If I change the timeout passed to debounce as 1, then the test passes. It seems to be that the fix you put in to prevent freezing when typing quickly is causing missed input when exiting a note too quickly :P.

It can be surmised that if processAllNow (called when unmounting) completes before debounce does, then the change to be added following completion of debounce will be lost. This is because the change is not added to the save action queue until after the 100 ms, and the component would have been already unmounted.

So I think to fix this, in onMarkdownEditorTextChange the change needs to be made without using debounce, but instead push a slightly modified makeSaveAction which waits the 100ms before before executing it's logic. There is a risk though that if doing the delay in the action, that you can get a backlog of save action queue items because the queue runs synchronously. But maybe this could be worked around by instead of waiting 100ms explicitly, record a timestamp 100ms after the current time in the action when creating it, then only apply a delay if that time has not yet passed

mrjo118 commented 1 day ago

Apologies, I jumped the gun a bit. After adding some additional logging and changing the code a bit, I can confirm that a note change scheduled using debounce does indeed execute, even after componentWillUnmount and the save action queue has completed. So that is not the issue. Looking at your old PR https://github.com/laurent22/joplin/pull/10134 , if in some cases it takes several seconds to work through the backlog of the save action queue, then indeed the user may have exited the note and then exited the app as well within that time. As soon as the user exits the app, any background processing will stop, so any items left in the queue will be lost. I confirmed this by adding logging at the end of the componentWillUnmount method, setting a delay of 10 seconds and writing another log in the callback. If I exit the note so the first log entry is written, then exit the app using the back button before the 10 seconds is up, the second log entry never gets written. The componentWillUnmount method is asynchronous so the user will be back on the list view before the processing completes.

Would it be possible with a React Native app to make some sort of visual indicator on the note screen while the note is saving, which persists onto the note list after exiting the note, until the save is completed? Or another option and maybe simpler would be to make the back button on the note (when switching from edit to to view mode) wait at intervals until the save action queue is empty, before executing the code to switch it to view mode. That way it will block the UI until all changes are saved

mrjo118 commented 7 hours ago

I made some further tests, and it turns out there are 3 issues: -Last editor input is lost if the app is closed before all queued changes have been saved - In manual testing of queued changes, I found that the 100 ms buffer between registering save actions greatly reduces the backlog and time required to execute it. As the the time to execute is unlikely to become significantly long, and large notes take a bit of time to render the note after exiting edit mode anyway, this is probably not that likely to occur providing that the user always exits the note before closing the app -Last editor input is lost if the user exits a note within 100 ms after making changes, where consecutive changes were made - I have attached a video below demonstrating this using the prebuilt Joplin 3.1.5 apk on Android emulator. The user would have to be super quick to make a change and exit both edit and view mode of the note for this occur, so again it's unlikely to happen. It's probably related to the fact that the note was unmounted before the change could be even finish rendering in view mode, so the state might not yet have been updated -Last editor input is sometimes lost when exiting the note editor after making changes - I have been able to fairly consistently reproduce this using the prebuilt Joplin 3.1.5 apk on Android emulator using a very large note by making text changes following pasting a chunk of text. I found that there is a specific error which appears in the logs during the period which I edit the note (which looks to align roughly with when I pasted the text) when the issue is reproduced. I've raised a new bug report with my findings for it https://github.com/laurent22/joplin/issues/11317. This could possibly be the cause of the intermittent lost input issues which Joplin has had for a long time

https://github.com/user-attachments/assets/84382c99-295e-4d47-8292-084e66292679

laurent22 / joplin