diff --git a/apps/weblibre/lib/features/geckoview/domain/providers/tab_state.dart b/apps/weblibre/lib/features/geckoview/domain/providers/tab_state.dart index 16753ef3..6d961969 100644 --- a/apps/weblibre/lib/features/geckoview/domain/providers/tab_state.dart +++ b/apps/weblibre/lib/features/geckoview/domain/providers/tab_state.dart @@ -73,6 +73,10 @@ class TabStates extends _$TabStates { _ => TabMode.regular, }; + // `current.parentId` still holds the last engine parent we applied, so + // capture whether the engine link changed before overwriting it below. + final engineParentChanged = contentState.parentId != current.parentId; + final newState = current.copyWith( parentId: contentState.parentId, contextId: contentState.contextId, @@ -87,6 +91,21 @@ class TabStates extends _$TabStates { state = {...state}..[contentState.id] = newState; + // Only reconcile DB hierarchy when the engine parent link actually changes. + // Content-state events also fire on every progress/title tick, and seeding + // opens a transaction, so re-running it on each tick would add needless DB + // I/O to this hot path. The debounced updateTabs pass is the backstop for + // anything not seeded here (e.g. a container assigned after the parent). + if (ref.mounted && engineParentChanged) { + await ref + .read(tabDataRepositoryProvider.notifier) + .seedParentFromEngineState( + childId: contentState.id, + parentId: contentState.parentId, + contextId: contentState.contextId, + ); + } + if (newState.isFinishedLoading) { ref .read(geckoInferenceRepositoryProvider.notifier) diff --git a/apps/weblibre/lib/features/geckoview/features/tabs/data/database/daos/tab.dart b/apps/weblibre/lib/features/geckoview/features/tabs/data/database/daos/tab.dart index c00715de..cec5a62e 100644 --- a/apps/weblibre/lib/features/geckoview/features/tabs/data/database/daos/tab.dart +++ b/apps/weblibre/lib/features/geckoview/features/tabs/data/database/daos/tab.dart @@ -172,29 +172,36 @@ class TabDao extends DatabaseAccessor with $TabDaoMixin { if (_pendingParentIds.isEmpty) return; final pendingChildren = selectOnly(db.tab) - ..addColumns([db.tab.id]) + ..addColumns([db.tab.id, db.tab.containerId]) ..where( db.tab.id.isIn(_pendingParentIds.keys) & db.tab.parentId.isNull() & db.tab.source.isNotValue(TabSource.manual.index), ); - final pendingChildIds = (await pendingChildren.get()) - .map((row) => row.read(db.tab.id)!) - .toSet(); + final pendingChildContainerIds = { + for (final row in await pendingChildren.get()) + row.read(db.tab.id)!: row.read(db.tab.containerId), + }; + final pendingChildIds = pendingChildContainerIds.keys.toSet(); if (pendingChildIds.isEmpty) { _pendingParentIds.clear(); return; } - final resolvableParents = await getExistingTabIds( - pendingChildIds.map((childId) => _pendingParentIds[childId]!).toSet(), - ).get().then((ids) => ids.toSet()); + final parentIds = pendingChildIds + .map((childId) => _pendingParentIds[childId]!) + .toSet(); + final resolvableParentContainerIds = await getTabsContainerId( + parentIds, + ).get().then(Map.fromEntries); await batch((batch) { for (final childId in pendingChildIds) { final parentId = _pendingParentIds[childId]; - if (parentId == null || !resolvableParents.contains(parentId)) { + if (parentId == null || + resolvableParentContainerIds[parentId] != + pendingChildContainerIds[childId]) { continue; } @@ -212,10 +219,65 @@ class TabDao extends DatabaseAccessor with $TabDaoMixin { _pendingParentIds.removeWhere( (childId, parentId) => !pendingChildIds.contains(childId) || - resolvableParents.contains(parentId), + resolvableParentContainerIds.containsKey(parentId), ); } + Future seedParentFromEngineState({ + required String childId, + required String? parentId, + required String? contextId, + }) { + return db.transaction(() async { + if (parentId == null || parentId == childId) { + // A null or self-referential engine parent can never seed a hierarchy + // link (the latter would create a cycle), so drop any pending retry. + _pendingParentIds.remove(childId); + return false; + } + + final child = await getTabDataById(childId).getSingleOrNull(); + if (child == null) { + _pendingParentIds[childId] = parentId; + return false; + } + if (child.parentId != null || child.source == TabSource.manual) { + _pendingParentIds.remove(childId); + return false; + } + + final parent = await getTabDataById(parentId).getSingleOrNull(); + if (parent == null) { + _pendingParentIds[childId] = parentId; + return false; + } + + final repairedContainerId = child.containerId == null && contextId != null + ? (await _containerIdsByContextualIdentity({contextId}))[contextId] + : null; + final effectiveChildContainerId = + repairedContainerId ?? child.containerId; + if (effectiveChildContainerId != parent.containerId) { + // The parent row exists but lives in a different container. Pending is + // only for parents that don't exist yet; a retry can't fix a container + // mismatch (mirrors updateTabs), so drop it rather than spin on it. + _pendingParentIds.remove(childId); + return false; + } + + await _updateByIdStatement(childId).write( + TabCompanion( + parentId: Value(parentId), + source: const Value(TabSource.manual), + containerId: + repairedContainerId.mapNotNull(Value.new) ?? const Value.absent(), + ), + ); + _pendingParentIds.remove(childId); + return true; + }); + } + Future _generateOrderKey({ required Value parentId, required Value containerId, @@ -1129,15 +1191,15 @@ class TabDao extends DatabaseAccessor with $TabDaoMixin { // inserts can seed the parent later without needing another parentId // change from Gecko. final parentSyncEligibleIds = next.isEmpty - ? const {} + ? const {} : await (() async { final query = selectOnly(db.tab) - ..addColumns([db.tab.id, db.tab.source]) + ..addColumns([db.tab.id, db.tab.source, db.tab.containerId]) ..where(db.tab.id.isIn(next.keys) & db.tab.parentId.isNull()); return { for (final row in await query.get()) if (row.readWithConverter(db.tab.source) != TabSource.manual) - row.read(db.tab.id)!, + row.read(db.tab.id)!: row.read(db.tab.containerId), }; })(); @@ -1149,7 +1211,7 @@ class TabDao extends DatabaseAccessor with $TabDaoMixin { // and aborting the whole batch. final parentIdsToValidate = { for (final state in next.values) - if (parentSyncEligibleIds.contains(state.id) && + if (parentSyncEligibleIds.containsKey(state.id) && state.parentId != null) state.parentId!, }; @@ -1183,9 +1245,14 @@ class TabDao extends DatabaseAccessor with $TabDaoMixin { if (containerIdsByContext[entry.value] case final containerId?) entry.key: containerId, }; + final parentContainerIds = parentIdsToValidate.isEmpty + ? const {} + : await getTabsContainerId( + parentIdsToValidate, + ).get().then(Map.fromEntries); for (final state in next.values) { - if (!parentSyncEligibleIds.contains(state.id)) { + if (!parentSyncEligibleIds.containsKey(state.id)) { _pendingParentIds.remove(state.id); continue; } @@ -1195,11 +1262,24 @@ class TabDao extends DatabaseAccessor with $TabDaoMixin { continue; } - if (existingParentIds.contains(state.parentId)) { - validatedParentIds[state.id] = state.parentId; + final parentId = state.parentId!; + if (parentId == state.id) { + // A self-referential engine parent would create a hierarchy cycle. _pendingParentIds.remove(state.id); + continue; + } + final effectiveChildContainerId = + repairedContainerIds[state.id] ?? parentSyncEligibleIds[state.id]; + final effectiveParentContainerId = + repairedContainerIds[parentId] ?? parentContainerIds[parentId]; + if (existingParentIds.contains(parentId) && + effectiveParentContainerId == effectiveChildContainerId) { + validatedParentIds[state.id] = parentId; + _pendingParentIds.remove(state.id); + } else if (!existingParentIds.contains(parentId)) { + _pendingParentIds[state.id] = parentId; } else { - _pendingParentIds[state.id] = state.parentId!; + _pendingParentIds.remove(state.id); } } diff --git a/apps/weblibre/lib/features/geckoview/features/tabs/domain/repositories/tab.dart b/apps/weblibre/lib/features/geckoview/features/tabs/domain/repositories/tab.dart index 3f36a500..55cc28d2 100644 --- a/apps/weblibre/lib/features/geckoview/features/tabs/domain/repositories/tab.dart +++ b/apps/weblibre/lib/features/geckoview/features/tabs/domain/repositories/tab.dart @@ -150,17 +150,26 @@ class TabDataRepository extends _$TabDataRepository { .setTabParent(tabId: tabId, newParentId: newParentId); } - Future promoteChildToParent(String childId) { + Future seedParentFromEngineState({ + required String childId, + required String? parentId, + required String? contextId, + }) { return ref .read(tabDatabaseProvider) .tabDao - .promoteChildToParent(childId); + .seedParentFromEngineState( + childId: childId, + parentId: parentId, + contextId: contextId, + ); } - Future moveTabAmongSiblings( - String tabId, { - required bool down, - }) { + Future promoteChildToParent(String childId) { + return ref.read(tabDatabaseProvider).tabDao.promoteChildToParent(childId); + } + + Future moveTabAmongSiblings(String tabId, {required bool down}) { return ref .read(tabDatabaseProvider) .tabDao diff --git a/apps/weblibre/test/drift/tabs/tab_hierarchy_order_test.dart b/apps/weblibre/test/drift/tabs/tab_hierarchy_order_test.dart index e347a742..f6c4caeb 100644 --- a/apps/weblibre/test/drift/tabs/tab_hierarchy_order_test.dart +++ b/apps/weblibre/test/drift/tabs/tab_hierarchy_order_test.dart @@ -1,5 +1,6 @@ import 'package:drift/drift.dart' show Value; import 'package:drift/native.dart'; +import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:lexo_rank/lexo_rank.dart'; import 'package:weblibre/data/database/functions/lexo_rank_functions.dart'; @@ -7,6 +8,7 @@ import 'package:weblibre/data/database/functions/url_functions.dart'; import 'package:weblibre/features/geckoview/domain/entities/states/tab.dart'; import 'package:weblibre/features/geckoview/features/tabs/data/database/database.dart'; import 'package:weblibre/features/geckoview/features/tabs/data/entities/tab_source.dart'; +import 'package:weblibre/features/geckoview/features/tabs/data/models/container_data.dart'; void main() { late TabDatabase db; @@ -112,6 +114,107 @@ void main() { expect(child.source, TabSource.manual); }); + test('engine parent seeding immediately claims an unclaimed row', () async { + await _insertTabs(db, const [ + _TabFixture('gecko-parent', source: TabSource.addedEvent), + _TabFixture('child', source: TabSource.addedEvent), + ]); + + final seeded = await db.tabDao.seedParentFromEngineState( + childId: 'child', + parentId: 'gecko-parent', + contextId: null, + ); + + final child = await db.tabDao.getTabDataById('child').getSingleOrNull(); + expect(seeded, isTrue); + expect(child, isNotNull); + expect(child!.parentId, 'gecko-parent'); + expect(child.source, TabSource.manual); + }); + + test('engine parent seeding rejects a self-referential parent', () async { + await _insertTabs(db, const [ + _TabFixture('tab', source: TabSource.addedEvent), + ]); + + final seeded = await db.tabDao.seedParentFromEngineState( + childId: 'tab', + parentId: 'tab', + contextId: null, + ); + + final tab = await db.tabDao.getTabDataById('tab').getSingleOrNull(); + expect(seeded, isFalse); + expect(tab, isNotNull); + expect(tab!.parentId, isNull); + expect(tab.source, TabSource.addedEvent); + }); + + test('engine parent seeding rejects cross-container parents', () async { + await _insertContainers(db, const [ + _ContainerFixture('parent-container', 'parent-context'), + _ContainerFixture('child-container', 'child-context'), + ]); + await _insertTabs(db, const [ + _TabFixture( + 'gecko-parent', + source: TabSource.addedEvent, + containerId: 'parent-container', + ), + _TabFixture( + 'child', + source: TabSource.addedEvent, + containerId: 'child-container', + ), + ]); + + final seeded = await db.tabDao.seedParentFromEngineState( + childId: 'child', + parentId: 'gecko-parent', + contextId: 'child-context', + ); + + final child = await db.tabDao.getTabDataById('child').getSingleOrNull(); + expect(seeded, isFalse); + expect(child, isNotNull); + expect(child!.parentId, isNull); + expect(child.source, TabSource.addedEvent); + }); + + test( + 'content-state sync validates parent against same-batch container repairs', + () async { + await _insertContainers(db, const [ + _ContainerFixture('container', 'context'), + ]); + await _insertTabs(db, const [ + _TabFixture('gecko-parent', source: TabSource.addedEvent), + _TabFixture('child', source: TabSource.addedEvent), + ]); + + await db.tabDao.updateTabs(null, { + 'gecko-parent': _tabState('gecko-parent', contextId: 'context'), + 'child': _tabState( + 'child', + parentId: 'gecko-parent', + contextId: 'context', + ), + }); + + final parent = await db.tabDao + .getTabDataById('gecko-parent') + .getSingleOrNull(); + final child = await db.tabDao.getTabDataById('child').getSingleOrNull(); + expect(parent, isNotNull); + expect(parent!.containerId, 'container'); + expect(child, isNotNull); + expect(child!.containerId, 'container'); + expect(child.parentId, 'gecko-parent'); + expect(child.source, TabSource.manual); + }, + ); + test( 'content-state sync retries an unresolved engine parent when the row arrives later', () async { @@ -335,11 +438,31 @@ Future _insertTabs(TabDatabase db, List<_TabFixture> tabs) async { tab.id, source: tab.source, parentId: Value(tab.parentId), + containerId: Value(tab.containerId), orderKey: Value(orderKeys[index]), ); } } +Future _insertContainers( + TabDatabase db, + List<_ContainerFixture> containers, +) async { + for (final container in containers) { + await db.containerDao.addContainer( + ContainerData( + id: container.id, + name: container.id, + color: Colors.blue, + orderKey: container.id, + metadata: ContainerMetadata.withDefaults( + contextualIdentity: container.contextId, + ), + ), + ); + } +} + Future> _orderedTabIds(TabDatabase db) { return db.tabDao.getAllTabIds().get(); } @@ -361,14 +484,28 @@ List _spacedOrderKeys(int count) { class _TabFixture { final String id; final String? parentId; + final String? containerId; final TabSource source; - const _TabFixture(this.id, {this.parentId, this.source = TabSource.manual}); + const _TabFixture( + this.id, { + this.parentId, + this.containerId, + this.source = TabSource.manual, + }); } -TabState _tabState(String id, {String? parentId}) { +class _ContainerFixture { + final String id; + final String contextId; + + const _ContainerFixture(this.id, this.contextId); +} + +TabState _tabState(String id, {String? parentId, String? contextId}) { return TabState.$default(id).copyWith( parentId: parentId, + contextId: contextId, url: Uri.parse('https://$id.example/'), title: id, );