Mobile: Fixes #10589: Fix selected note changes on moving to a different folder (#10630)

.eslintignore

@@ -964,6 +964,7 @@ packages/lib/ntp.js
 packages/lib/onedrive-api.test.js
 packages/lib/onedrive-api.js
 packages/lib/path-utils.js
+packages/lib/reducer.test.js
 packages/lib/reducer.js
 packages/lib/registry.test.js
 packages/lib/registry.js

.gitignore

@@ -943,6 +943,7 @@ packages/lib/ntp.js
 packages/lib/onedrive-api.test.js
 packages/lib/onedrive-api.js
 packages/lib/path-utils.js
+packages/lib/reducer.test.js
 packages/lib/reducer.js
 packages/lib/registry.test.js
 packages/lib/registry.js

packages/app-mobile/components/screens/Note.tsx

@@ -1399,15 +1399,18 @@ class NoteScreenComponent extends BaseScreenComponent<Props, State> implements B
 		}, 50);
 	}
 
-	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
+	private async folderPickerOptions_valueChanged(itemValue: string) {
-	private async folderPickerOptions_valueChanged(itemValue: any) {
 		const note = this.state.note;
 		const isProvisionalNote = this.props.provisionalNoteIds.includes(note.id);
 
 		if (isProvisionalNote) {
 			await this.saveNoteButton_press(itemValue);
 		} else {
-			await Note.moveToFolder(note.id, itemValue);
+			await Note.moveToFolder(
+				note.id,
+				itemValue,
+				{ dispatchOptions: { preserveSelection: true } },
+			);
 		}
 
 		note.parent_id = itemValue;
@@ -1428,7 +1431,13 @@ class NoteScreenComponent extends BaseScreenComponent<Props, State> implements B
 			onValueChange: this.folderPickerOptions_valueChanged,
 		};
 
-		if (this.folderPickerOptions_ && options.selectedFolderId === this.folderPickerOptions_.selectedFolderId) return this.folderPickerOptions_;
+		if (
+			this.folderPickerOptions_
+			&& options.selectedFolderId === this.folderPickerOptions_.selectedFolderId
+			&& options.enabled === this.folderPickerOptions_.enabled
+		) {
+			return this.folderPickerOptions_;
+		}
 
 		this.folderPickerOptions_ = options;
 		return this.folderPickerOptions_;
@@ -1683,7 +1692,6 @@ const NoteScreen = connect((state: AppState) => {
 	return {
 		noteId: state.selectedNoteIds.length ? state.selectedNoteIds[0] : null,
 		noteHash: state.selectedNoteHash,
-		folderId: state.selectedFolderId,
 		itemType: state.selectedItemType,
 		folders: state.folders,
 		searchQuery: state.searchQuery,

packages/lib/components/shared/note-screen-shared.ts

@@ -102,6 +102,7 @@ shared.saveNoteButton_press = async function(comp: BaseNoteScreenComponent, fold
 	const saveOptions = {
 		userSideValidation: true,
 		fields: BaseModel.diffObjectsFields(comp.state.lastSavedNote, note),
+		dispatchOptions: { preserveSelection: true },
 	};
 
 	const hasAutoTitle = comp.state.newAndNoTitleChangeNoteId || (isProvisionalNote && !note.title);

packages/lib/models/Note.ts

@@ -624,7 +624,7 @@ export default class Note extends BaseItem {
 		});
 	}
 
-	public static async moveToFolder(noteId: string, folderId: string) {
+	public static async moveToFolder(noteId: string, folderId: string, saveOptions: SaveOptions|null = null) {
 		if (folderId === this.getClass('Folder').conflictFolderId()) throw new Error(_('Cannot move note to "%s" notebook', this.getClass('Folder').conflictFolderTitle()));
 
 		// When moving a note to a different folder, the user timestamp is not
@@ -643,7 +643,7 @@ export default class Note extends BaseItem {
 			updated_time: time.unixMs(),
 		};
 
-		return Note.save(modifiedNote, { autoTimestamp: false });
+		return Note.save(modifiedNote, { autoTimestamp: false, ...saveOptions });
 	}
 
 	public static changeNoteType(note: NoteEntity, type: string) {
@@ -841,6 +841,7 @@ export default class Note extends BaseItem {
 				provisional: isProvisional,
 				ignoreProvisionalFlag: ignoreProvisionalFlag,
 				changedFields: changedFields,
+				...options?.dispatchOptions,
 			});
 		}
 

packages/lib/models/utils/types.ts

@@ -49,6 +49,7 @@ export interface SaveOptions {
 	provisional?: boolean;
 	ignoreProvisionalFlag?: boolean;
 	dispatchUpdateAction?: boolean;
+	dispatchOptions?: { preserveSelection: boolean };
 	changeSource?: number;
 	disableReadOnlyCheck?: boolean;
 }

packages/lib/reducer.test.ts

@@ -1,9 +1,11 @@
-const { setupDatabaseAndSynchronizer, switchClient, createNTestNotes, createNTestFolders, createNTestTags } = require('./testing/test-utils.js');
+import { setupDatabaseAndSynchronizer, switchClient, createNTestNotes, createNTestFolders, createNTestTags } from './testing/test-utils';
-const reducer = require('./reducer').default;
+import reducer, { defaultState, MAX_HISTORY, State } from './reducer';
-const { defaultState, MAX_HISTORY } = require('./reducer');
+import { BaseItemEntity, FolderEntity, NoteEntity, TagEntity } from './services/database/types';
+import Note from './models/Note';
+import BaseModel from './BaseModel';
 // const { ALL_NOTES_FILTER_ID } = require('./reserved-ids');
 
-function initTestState(folders, selectedFolderIndex, notes, selectedNoteIndexes, tags = null, selectedTagIndex = null) {
+function initTestState(folders: FolderEntity[], selectedFolderIndex: number, notes: NoteEntity[], selectedNoteIndexes: number[], tags: TagEntity[] = null, selectedTagIndex: number = null) {
 	let state = defaultState;
 
 	if (selectedFolderIndex !== null) {
@@ -32,7 +34,7 @@ function initTestState(folders, selectedFolderIndex, notes, selectedNoteIndexes,
 	return state;
 }
 
-function goToNote(notes, selectedNoteIndexes, state) {
+function goToNote(notes: NoteEntity[], selectedNoteIndexes: number[], state: State) {
 	if (selectedNoteIndexes !== null) {
 		const selectedIds = [];
 		for (let i = 0; i < selectedNoteIndexes.length; i++) {
@@ -43,7 +45,7 @@ function goToNote(notes, selectedNoteIndexes, state) {
 	return state;
 }
 
-function goBackWard(state) {
+function goBackWard(state: State) {
 	if (!state.backwardHistoryNotes.length)	return state;
 	state = reducer(state, {
 		type: 'HISTORY_BACKWARD',
@@ -51,7 +53,7 @@ function goBackWard(state) {
 	return state;
 }
 
-function goForward(state) {
+function goForward(state: State) {
 	if (!state.forwardHistoryNotes.length)	return state;
 	state = reducer(state, {
 		type: 'HISTORY_FORWARD',
@@ -59,8 +61,8 @@ function goForward(state) {
 	return state;
 }
 
-function createExpectedState(items, keepIndexes, selectedIndexes) {
+function createExpectedState(items: BaseItemEntity[], keepIndexes: number[], selectedIndexes: number[]) {
-	const expected = { items: [], selectedIds: [] };
+	const expected = { items: [] as BaseItemEntity[], selectedIds: [] as string[] };
 
 	for (let i = 0; i < selectedIndexes.length; i++) {
 		expected.selectedIds.push(items[selectedIndexes[i]].id);
@@ -71,7 +73,7 @@ function createExpectedState(items, keepIndexes, selectedIndexes) {
 	return expected;
 }
 
-function getIds(items, indexes = null) {
+function getIds(items: BaseItemEntity[], indexes: number[]|null = null) {
 	const ids = [];
 	for (let i = 0; i < items.length; i++) {
 		if (!indexes || i in indexes) {
@@ -86,7 +88,6 @@ describe('reducer', () => {
 	beforeEach(async () => {
 		await setupDatabaseAndSynchronizer(1);
 		await switchClient(1);
-
 	});
 
 	// tests for NOTE_DELETE
@@ -301,7 +302,7 @@ describe('reducer', () => {
 	// tests for TAG_DELETE
 	it('should delete selected tag', (async () => {
 		const tags = await createNTestTags(5);
-		let state = initTestState(null, null, null, null, tags, [2]);
+		let state = initTestState(null, null, null, null, tags, 2);
 
 		// test action
 		state = reducer(state, { type: 'TAG_DELETE', id: tags[2].id });
@@ -314,7 +315,7 @@ describe('reducer', () => {
 
 	it('should delete tag when a tag above is selected', (async () => {
 		const tags = await createNTestTags(5);
-		let state = initTestState(null, null, null, null, tags, [2]);
+		let state = initTestState(null, null, null, null, tags, 2);
 
 		// test action
 		state = reducer(state, { type: 'TAG_DELETE', id: tags[4].id });
@@ -327,7 +328,7 @@ describe('reducer', () => {
 
 	it('should delete tag when a tag below is selected', (async () => {
 		const tags = await createNTestTags(5);
-		let state = initTestState(null, null, null, null, tags, [2]);
+		let state = initTestState(null, null, null, null, tags, 2);
 
 		// test action
 		state = reducer(state, { type: 'TAG_DELETE', id: tags[0].id });
@@ -616,7 +617,7 @@ describe('reducer', () => {
 		const oldTags = tags.slice(0, 5);
 		{
 			// Case 1. The input which is deep equal to the current state.tags doesn't change state.tags.
-			const oldState = initTestState(null, null, null, null, oldTags, [2]);
+			const oldState = initTestState(null, null, null, null, oldTags, 2);
 			const newTags = oldTags.slice();
 			// test action
 			const newState = reducer(oldState, { type: 'TAG_UPDATE_ALL', items: newTags });
@@ -624,11 +625,47 @@ describe('reducer', () => {
 		}
 		{
 			// Case 2. A different input changes state.tags.
-			const oldState = initTestState(null, null, null, null, oldTags, [2]);
+			const oldState = initTestState(null, null, null, null, oldTags, 2);
 			const newTags = oldTags.slice().splice(3, 1, tags[5]);
 			// test action
 			const newState = reducer(oldState, { type: 'TAG_UPDATE_ALL', items: newTags });
 			expect(newState.tags).not.toBe(oldState.tags);
 		}
 	});
+
+	// Regression test for #10589.
+	it.each([
+		true, false,
+	])('should preserve note selection if specified while moving a note (preserveSelection: %j)', async (preserveSelection) => {
+		const folders = await createNTestFolders(3);
+		const notes = await createNTestNotes(5, folders[0]);
+
+		// select the 1st folder and the 1st note
+		let state = initTestState(folders, 0, notes, [0]);
+		state = goToNote(notes, [0], state);
+
+		expect(state.selectedNoteIds).toHaveLength(1);
+
+		BaseModel.dispatch = jest.fn((action: unknown) => {
+			state = reducer(state, action);
+		});
+
+		// Dispatching with preserveSelection should preserve the selected note (as is done on
+		// mobile).
+		await Note.moveToFolder(
+			state.selectedNoteIds[0],
+			folders[1].id,
+			preserveSelection ? { dispatchOptions: { preserveSelection: true } } : undefined,
+		);
+
+		expect(BaseModel.dispatch).toHaveBeenCalled();
+		if (preserveSelection) {
+			expect(state.selectedNoteIds).toMatchObject([notes[0].id]);
+		} else {
+			expect(state.selectedNoteIds).toMatchObject([notes[1].id]);
+		}
+		// Original note should no longer be present in the sidebar
+		expect(state.notes.every(n => n.id !== notes[0].id)).toBe(true);
+		expect(state.selectedFolderId).toBe(folders[0].id);
+	});
 });

packages/lib/reducer.ts

@@ -1005,7 +1005,11 @@ const reducer = produce((draft: Draft<State> = defaultState, action: any) => {
 
 				draft.notes = newNotes;
 
-				if (noteFolderHasChanged) {
+				// Ensure that the selected note is still in the current folder.
+				// For example, if the user drags the current note to a different folder,
+				// a new note should be selected.
+				// In some cases, however, the selection needs to be preserved (e.g. the mobile app).
+				if (noteFolderHasChanged && !action.preserveSelection) {
 					let newIndex = movedNotePreviousIndex;
 					if (newIndex >= newNotes.length) newIndex = newNotes.length - 1;
 					if (!newNotes.length) newIndex = -1;