Code review

Author: Keavon

desktop/src/app.rs

@@ -315,23 +315,17 @@ impl App {
 				}
 			}
 			DesktopFrontendMessage::PersistenceLoadDocuments => {
-				let current_id = self.persistent_data.current_document_id();
-				let mut seen_current = false;
-
+				// Open all documents in persisted tab order, then select the current one
 				for (id, document) in self.persistent_data.all_documents() {
-					let is_current = current_id == Some(id);
-					let to_front = !seen_current && !is_current;
-					seen_current |= is_current;
-
 					responses.push(DesktopWrapperMessage::LoadDocument {
 						id,
 						document,
-						to_front,
+						to_front: false,
 						select_after_open: false,
 					});
 				}
 
-				if let Some(id) = current_id {
+				if let Some(id) = self.persistent_data.current_document_id() {
 					responses.push(DesktopWrapperMessage::SelectDocument { id });
 				}
 			}

desktop/src/persist.rs

@@ -30,7 +30,7 @@ impl PersistentData {
 		}
 
 		if let Some(order) = &self.document_order {
-			self.force_order(order.clone());
+			self.force_order(&order.clone());
 		}
 		self.flush();
 	}
@@ -65,7 +65,7 @@ impl PersistentData {
 	}
 
 	pub(crate) fn force_document_order(&mut self, order: Vec<DocumentId>) {
-		self.force_order(order.clone());
+		self.force_order(&order);
 		self.document_order = Some(order);
 		self.flush();
 	}
@@ -83,9 +83,9 @@ impl PersistentData {
 	}
 
 	// Reorders the documents array to match a desired ordering, keeping unmentioned documents at the end
-	fn force_order(&mut self, desired_order: Vec<DocumentId>) {
+	fn force_order(&mut self, desired_order: &[DocumentId]) {
 		let mut ordered_prefix_length = 0;
-		for id in &desired_order {
+		for id in desired_order {
 			if let Some(offset) = self.documents[ordered_prefix_length..].iter().position(|doc| doc.id == *id) {
 				let found_index = ordered_prefix_length + offset;
 				if found_index != ordered_prefix_length {
@@ -132,6 +132,7 @@ impl PersistentData {
 		*self = loaded;
 
 		self.garbage_collect_document_files();
+		Self::delete_old_cef_browser_directory();
 	}
 
 	// Remove orphaned document content files that have no corresponding entry in the persisted state
@@ -158,6 +159,14 @@ impl PersistentData {
 		}
 	}
 
+	// TODO: Eventually remove this cleanup code for the old "browser" CEF directory (renamed to "cef")
+	fn delete_old_cef_browser_directory() {
+		let old_browser_dir = crate::dirs::app_data_dir().join("browser");
+		if old_browser_dir.is_dir() {
+			let _ = std::fs::remove_dir_all(&old_browser_dir);
+		}
+	}
+
 	fn state_file_path() -> std::path::PathBuf {
 		let mut path = crate::dirs::app_data_dir();
 		path.push(crate::consts::APP_STATE_FILE_NAME);

frontend/src/utility-functions/persistence.ts

@@ -16,6 +16,25 @@ function createDocumentInfo(id: bigint, name: string, isSaved: boolean): Persist
 	return { id, name, is_saved: isSaved };
 }
 
+// Reorder document entries to match the given ID ordering, appending any unmentioned entries at the end
+function reorderDocuments(documents: PersistedDocumentInfo[], orderedIds: bigint[]): PersistedDocumentInfo[] {
+	const byId = new Map(documents.map((entry) => [entry.id, entry]));
+	const reordered: PersistedDocumentInfo[] = [];
+
+	orderedIds.forEach((id) => {
+		const existing = byId.get(id);
+		if (existing) {
+			reordered.push(existing);
+			byId.delete(id);
+		}
+	});
+
+	// Append any entries not yet present in the portfolio (e.g. documents still loading at startup)
+	byId.forEach((entry) => reordered.push(entry));
+
+	return reordered;
+}
+
 // ====================================
 // State-based persistence (new format)
 // ====================================
@@ -26,22 +45,7 @@ export async function storeDocumentTabOrder(portfolio: PortfolioStore) {
 
 	await databaseUpdate<PersistedState>("state", (old) => {
 		const state = old || emptyPersistedState();
-
-		// Reorder existing document entries to match the portfolio's tab order, preserving metadata
-		const byId = new Map(state.documents.map((entry) => [entry.id, entry]));
-		const reordered: PersistedDocumentInfo[] = [];
-		orderedIds.forEach((id) => {
-			const existing = byId.get(id);
-			if (existing) {
-				reordered.push(existing);
-				byId.delete(id);
-			}
-		});
-
-		// Append any entries not yet present in the portfolio (e.g. documents still loading at startup)
-		byId.forEach((entry) => reordered.push(entry));
-
-		return { ...state, documents: reordered };
+		return { ...state, documents: reorderDocuments(state.documents, orderedIds) };
 	});
 }
 
@@ -71,23 +75,9 @@ export async function storeDocument(autoSaveDocument: MessageBody<"TriggerPersis
 			state.documents.push(entry);
 		}
 
-		// Reorder to match the portfolio's tab order
-		const byId = new Map(state.documents.map((doc) => [doc.id, doc]));
-		const reordered: PersistedDocumentInfo[] = [];
-		orderedIds.forEach((id) => {
-			const existing = byId.get(id);
-			if (existing) {
-				reordered.push(existing);
-				byId.delete(id);
-			}
-		});
-
-		// Append any entries not yet present in the portfolio (e.g. documents still loading at startup)
-		byId.forEach((entry) => reordered.push(entry));
-
 		// eslint-disable-next-line camelcase
 		state.current_document = documentId;
-		state.documents = reordered;
+		state.documents = reorderDocuments(state.documents, orderedIds);
 		return state;
 	});
 }
@@ -131,15 +121,13 @@ export async function loadDocuments(editor: EditorWrapper) {
 	const currentId = state.current_document;
 	const currentEntry = currentId !== undefined ? state.documents.find((doc) => doc.id === currentId) : undefined;
 	const current = currentEntry || state.documents[state.documents.length - 1];
-	const currentIndex = state.documents.indexOf(current);
 
-	// Open documents in order around the current document, placing earlier ones before it and later ones after
-	state.documents.forEach((entry, index) => {
+	// Open all documents in persisted tab order, then select the current one
+	state.documents.forEach((entry) => {
 		const content = documentContents[String(entry.id)];
 		if (content === undefined) return;
 
-		const toFront = index < currentIndex;
-		editor.openAutoSavedDocument(entry.id, entry.name, entry.is_saved, content, toFront);
+		editor.openAutoSavedDocument(entry.id, entry.name, entry.is_saved, content, false);
 	});
 
 	editor.selectDocument(current.id);
@@ -252,9 +240,9 @@ async function migrateToNewFormat() {
 			let name = "";
 			if ("name" in details && typeof details.name === "string") name = details.name;
 
-			const status = savedStatusFromUnknown(details);
+			const isSaved = extractIsSavedFromUnknown(details);
 
-			newDocumentInfos.push(createDocumentInfo(id, name, status.isSaved));
+			newDocumentInfos.push(createDocumentInfo(id, name, isSaved));
 		});
 	}
 
@@ -271,20 +259,16 @@ async function migrateToNewFormat() {
 }
 
 // TODO: Eventually remove this document upgrade code
-function savedStatusFromUnknown(details: unknown): { isSaved: boolean; isAutoSaved: boolean } {
-	if (typeof details !== "object" || details === null) return { isSaved: false, isAutoSaved: false };
+function extractIsSavedFromUnknown(details: unknown): boolean {
+	if (typeof details !== "object" || details === null) return false;
 
 	// Old camelCase format
-	if ("isSaved" in details && "isAutoSaved" in details) {
-		return { isSaved: Boolean(details.isSaved), isAutoSaved: Boolean(details.isAutoSaved) };
-	}
+	if ("isSaved" in details) return Boolean(details.isSaved);
 
 	// New snake_case format
-	if ("is_saved" in details && "is_auto_saved" in details) {
-		return { isSaved: Boolean(details.is_saved), isAutoSaved: Boolean(details.is_auto_saved) };
-	}
+	if ("is_saved" in details) return Boolean(details.is_saved);
 
-	return { isSaved: false, isAutoSaved: false };
+	return false;
 }
 
 // =================