From 1b44dc0fe1df195f9c1f380945b6f568bc237eed Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Sat, 5 Sep 2026 16:28:57 -0400 Subject: [PATCH] fix(mobile): fence send preparation by source draft visit Signed-off-by: Logan Johnson --- .../compose_bar/compose_bar_widget.dart | 166 +++++++++++++----- .../channels/compose_bar/draft_lifecycle.dart | 20 ++- .../channels/compose_bar/helpers.dart | 22 ++- .../features/channels/compose_bar_test.dart | 17 +- .../send_lifecycle_tests.dart | 152 ++++++++++++++++ 5 files changed, 318 insertions(+), 59 deletions(-) create mode 100644 mobile/test/features/channels/compose_bar_test/send_lifecycle_tests.dart diff --git a/mobile/lib/features/channels/compose_bar/compose_bar_widget.dart b/mobile/lib/features/channels/compose_bar/compose_bar_widget.dart index ca34ffd2a1..a98c94fd81 100644 --- a/mobile/lib/features/channels/compose_bar/compose_bar_widget.dart +++ b/mobile/lib/features/channels/compose_bar/compose_bar_widget.dart @@ -41,6 +41,10 @@ class ComposeBar extends HookConsumerWidget { final draftKey = composeDraftKey(channelId, threadHeadId: threadHeadId); final draftRevision = useRef(0); final draftIdentity = _composerDraftIdentity(ref); + // A -> B -> A is a new visit, even when the eventual draft key is equal. + final visit = useMemoized(Object.new, [draftIdentity, draftKey]); + final currentVisit = useRef(visit)..value = visit; + final activeSend = useRef(null); final isComposerExpanded = useState(false); final androidImeTransitionStarted = useState( defaultTargetPlatform != TargetPlatform.android, @@ -220,6 +224,10 @@ class ComposeBar extends HookConsumerWidget { // mentions. Used to pass resolved pubkeys directly to onSend and to attach // selected non-member agents before the message is published. final mentionMap = useRef({}); + useEffect(() { + mentionMap.value.clear(); + return null; + }, [visit]); // Channel autocomplete state ---------------------------------------------- final channelQuery = useState(null); @@ -465,50 +473,69 @@ class ComposeBar extends HookConsumerWidget { return; } final submittedDraftRevision = draftRevision.value; + final attempt = Object(); + activeSend.value = attempt; + isSending.value = true; + bool ownsSource() => + context.mounted && + identical(currentVisit.value, visit) && + '${ref.read(relayConfigProvider).baseUrl}' + ':${ref.read(myPubkeyProvider) ?? 'anon'}' == + draftIdentity; + void ensureCurrent() { + if (!ownsSource()) throw const _ComposeSendCancelled(); + } + // Resolved before any await: see // `_reportSendCancelledByCommunitySwitch`. final messenger = ScaffoldMessenger.maybeOf(context); - // Extract pubkeys for mentions present in the final text. - final selectedMentions = [ - for (final entry in mentionMap.value.entries) - if (hasMention(text, entry.key)) entry.value, - ]; - final outgoing = _OutgoingMentions(selectedMentions); - final scan = await _scanNonMemberMentions( - ref, - channelId: channelId, - selectedMentions: selectedMentions, - currentPubkey: currentPubkey, - ); - - // Mentioning humans outside the channel prompts "Invite" / "Do - // nothing" (send without inviting) — mirrors desktop's - // NonMemberMentionDialog. Agents keep the existing silent auto-add. - if (scan.humans.isNotEmpty) { - if (!context.mounted) return; - final choice = await _promptNonMemberMention( - context, - names: [for (final candidate in scan.humans) candidate.label], - canInvite: scan.canAddMembers, - ); - if (choice == null) return; // Dismissed — keep the draft, send nothing. - outgoing.resolveHumanChoice(choice, scan.humans); - } - - final queuedAttachments = List<_PendingAttachment>.of(attachments.value); - final channelActions = ref.read(channelActionsProvider); - - // An add that was refused doesn't block the message: it is reported and - // the un-added mentions are demoted to reference tags so the send lands. - Future addMentionedNonMembers() => outgoing.addNonMembers( - channelActions, - scan: scan, - messenger: messenger, - ); - - isSending.value = true; try { + // Extract pubkeys for mentions present in the final text. + final selectedMentions = [ + for (final entry in mentionMap.value.entries) + if (hasMention(text, entry.key)) entry.value, + ]; + final outgoing = _OutgoingMentions(selectedMentions); + final scan = await _scanNonMemberMentions( + ref, + channelId: channelId, + selectedMentions: selectedMentions, + currentPubkey: currentPubkey, + ); + ensureCurrent(); + if (draftRevision.value != submittedDraftRevision) return; + + // Mentioning humans outside the channel prompts "Invite" / "Do + // nothing" (send without inviting) — mirrors desktop's + // NonMemberMentionDialog. Agents keep the existing silent auto-add. + if (scan.humans.isNotEmpty) { + if (!context.mounted) return; + final choice = await _promptNonMemberMention( + context, + names: [for (final candidate in scan.humans) candidate.label], + canInvite: scan.canAddMembers, + ); + ensureCurrent(); + if (choice == null || draftRevision.value != submittedDraftRevision) { + return; // Dismissal or newer intent keeps the draft, sends nothing. + } + outgoing.resolveHumanChoice(choice, scan.humans); + } + + final queuedAttachments = List<_PendingAttachment>.of( + attachments.value, + ); + final channelActions = ref.read(channelActionsProvider); + + // An add that was refused doesn't block the message: it is reported and + // the un-added mentions are demoted to reference tags so the send lands. + Future addMentionedNonMembers() => outgoing.addNonMembers( + channelActions, + scan: scan, + messenger: messenger, + ensureCurrent: ensureCurrent, + ); if (queuedAttachments.isEmpty) { if (!context.mounted) return; await _sendTextOnlyDraft( @@ -517,6 +544,7 @@ class ComposeBar extends HookConsumerWidget { mentionMap: mentionMap, draftRevision: draftRevision, submittedDraftRevision: submittedDraftRevision, + ownsSource: ownsSource, focusNode: focusNode, clearComposer: clearComposer, addMentionedNonMembers: addMentionedNonMembers, @@ -537,8 +565,16 @@ class ComposeBar extends HookConsumerWidget { final draftMentions = Map.of( mentionMap.value, ); - clearComposer(); - final clearedDraftRevision = draftRevision.value; + // Membership preparation is cancellable intent, not a detached send. + // Keep that draft recoverable until preparation succeeds. Ordinary + // member-only media keeps the existing background delivery behavior. + final preparingMembership = + scan.agentPubkeys.isNotEmpty || scan.humans.isNotEmpty; + int? clearedDraftRevision; + if (!preparingMembership) { + clearComposer(); + clearedDraftRevision = draftRevision.value; + } uploadingCount.value += 1; uploadProgress.value = 0; isSending.value = false; @@ -557,7 +593,8 @@ class ComposeBar extends HookConsumerWidget { uploadService, attachment, onProgress: (progress) { - if (context.mounted) { + if (context.mounted && + queueGeneration == uploadGeneration.value) { uploadProgress.value = (index + progress) / queuedAttachments.length; } @@ -566,7 +603,8 @@ class ComposeBar extends HookConsumerWidget { ); if (queueGeneration != uploadGeneration.value) return; uploaded.add(descriptor); - if (context.mounted) { + if (context.mounted && + queueGeneration == uploadGeneration.value) { uploadProgress.value = (index + 1) / queuedAttachments.length; } } @@ -576,8 +614,18 @@ class ComposeBar extends HookConsumerWidget { customEmoji: customEmoji, ); if (queueGeneration != uploadGeneration.value) return; + if (preparingMembership) { + ensureCurrent(); + if (draftRevision.value != submittedDraftRevision) return; + } await addMentionedNonMembers(); if (queueGeneration != uploadGeneration.value) return; + if (preparingMembership && + ownsSource() && + draftRevision.value == submittedDraftRevision) { + clearComposer(); + clearedDraftRevision = draftRevision.value; + } await delivery( payload.content, outgoing.pubkeys, @@ -585,8 +633,8 @@ class ComposeBar extends HookConsumerWidget { ); } catch (error) { if (cancellation.isCancelled) return; - if (context.mounted) uploadError.value = _formatUploadError(error); - if (context.mounted && + if (ownsSource()) uploadError.value = _formatUploadError(error); + if (ownsSource() && queueGeneration == uploadGeneration.value && draftRevision.value == clearedDraftRevision) { controller.value = draftText; @@ -598,7 +646,12 @@ class ComposeBar extends HookConsumerWidget { focusNode.requestFocus(); } } finally { - if (!retainedForRetry) { + // An uncleared source still owns these files (including on error). + final sourceRetainsFiles = + preparingMembership && + clearedDraftRevision == null && + ownsSource(); + if (!retainedForRetry && !sourceRetainsFiles) { await _deleteOwnedAttachments(queuedAttachments); } if (activeUploadCancellation.value == cancellation) { @@ -609,8 +662,27 @@ class ComposeBar extends HookConsumerWidget { } } }()); + } on _ComposeSendCancelled { + // The source draft was never cleared during preparation. + } on StateError catch (error) { + if (ownsSource()) { + messenger?.showSnackBar( + SnackBar(content: Text(_composeSendErrorMessage(error))), + ); + } else { + _reportSendCancelledByCommunitySwitch(messenger); + } + } catch (error) { + if (ownsSource()) { + messenger?.showSnackBar( + SnackBar(content: Text(_composeSendErrorMessage(error))), + ); + } } finally { - if (context.mounted && isSending.value) isSending.value = false; + if (ownsSource() && identical(activeSend.value, attempt)) { + activeSend.value = null; + isSending.value = false; + } } } diff --git a/mobile/lib/features/channels/compose_bar/draft_lifecycle.dart b/mobile/lib/features/channels/compose_bar/draft_lifecycle.dart index b712091b71..0838f47f20 100644 --- a/mobile/lib/features/channels/compose_bar/draft_lifecycle.dart +++ b/mobile/lib/features/channels/compose_bar/draft_lifecycle.dart @@ -1,11 +1,16 @@ part of '../compose_bar.dart'; +class _ComposeSendCancelled implements Exception { + const _ComposeSendCancelled(); +} + Future _sendTextOnlyDraft({ required BuildContext context, required _MarkdownEditingController controller, required ObjectRef> mentionMap, required ObjectRef draftRevision, required int submittedDraftRevision, + required bool Function() ownsSource, required FocusNode focusNode, required VoidCallback clearComposer, required Future Function() addMentionedNonMembers, @@ -19,7 +24,7 @@ Future _sendTextOnlyDraft({ int? clearedDraftRevision; void restoreClearedDraft() { - if (!context.mounted || + if (!ownsSource() || clearedDraftText == null || clearedDraftMentions == null || clearedDraftRevision == null || @@ -35,6 +40,7 @@ Future _sendTextOnlyDraft({ try { await addMentionedNonMembers(); + if (!ownsSource()) return; // Clear before optimistic insertion so the outgoing row and draft never // appear simultaneously during the send transition. If the user edited // while membership changes were pending, preserve that newer draft. @@ -49,6 +55,8 @@ Future _sendTextOnlyDraft({ outgoing.pubkeys, mediaTags: [...payload.mediaTags, ...outgoing.referenceTags], ); + } on _ComposeSendCancelled { + restoreClearedDraft(); } on StateError { restoreClearedDraft(); _reportSendCancelledByCommunitySwitch(messenger); @@ -80,12 +88,19 @@ void _useComposeDraftLifecycle({ required _IOSAttachmentPopoverController iosAttachmentPopover, required VoidCallback onDraftIdentityChanged, }) { + // Hook disposal can lag the replacement effect. An old text listener must + // not persist the incoming draft into its previous channel/account scope. + final owner = useMemoized(Object.new, [draftKey, draftIdentity]); + final currentOwner = useRef(owner)..value = owner; final lastDraftIdentity = useRef(null); + final lastDraftKey = useRef(null); useEffect(() { final identityChanged = lastDraftIdentity.value != null && - lastDraftIdentity.value != draftIdentity; + (lastDraftIdentity.value != draftIdentity || + lastDraftKey.value != draftKey); lastDraftIdentity.value = draftIdentity; + lastDraftKey.value = draftKey; final saved = ref.read(composeDraftsProvider.notifier).textFor(draftKey); if (identityChanged) { draftRevision.value += 1; @@ -108,6 +123,7 @@ void _useComposeDraftLifecycle({ var lastPersistedText = controller.text; void persistDraft() { + if (!identical(currentOwner.value, owner)) return; final text = controller.text; if (text == lastPersistedText) return; lastPersistedText = text; diff --git a/mobile/lib/features/channels/compose_bar/helpers.dart b/mobile/lib/features/channels/compose_bar/helpers.dart index fa20df6d16..a65df6bafa 100644 --- a/mobile/lib/features/channels/compose_bar/helpers.dart +++ b/mobile/lib/features/channels/compose_bar/helpers.dart @@ -447,10 +447,11 @@ Future<_NonMemberAddOutcome> _addMentionedNonMembers( required List agentPubkeys, required List humanPubkeys, required bool canAddMembers, + required VoidCallback ensureCurrent, }) async { final pending = [ - if (agentPubkeys.isNotEmpty) (agentPubkeys, 'bot'), - if (humanPubkeys.isNotEmpty) (humanPubkeys, 'member'), + for (final pubkey in agentPubkeys) ([pubkey], 'bot'), + for (final pubkey in humanPubkeys) ([pubkey], 'member'), ]; if (pending.isEmpty) return _NonMemberAddOutcome.empty; @@ -467,11 +468,15 @@ Future<_NonMemberAddOutcome> _addMentionedNonMembers( final errors = []; for (final (pubkeys, role) in pending) { try { + ensureCurrent(); await channelActions.addMembers( channelId: channelId, pubkeys: pubkeys, role: role, ); + ensureCurrent(); + } on _ComposeSendCancelled { + rethrow; } on StateError { rethrow; } catch (error) { @@ -518,12 +523,13 @@ Future<_NonMemberMentionScan> _scanNonMemberMentions( ); if (selectedMentions.isEmpty) return none; - final channel = (await ref.read( - channelsProvider.future, - )).firstWhere((candidate) => candidate.id == channelId); + // Capture both reads before yielding: WidgetRef may be disposed while waiting. + final (channels, members) = await ( + ref.read(channelsProvider.future), + ref.read(channelMembersProvider(channelId).future), + ).wait; + final channel = channels.firstWhere((candidate) => candidate.id == channelId); if (channel.isDm) return none; - - final members = await ref.read(channelMembersProvider(channelId).future); final memberPubkeys = { for (final member in members) member.pubkey.toLowerCase(), }; @@ -607,6 +613,7 @@ class _OutgoingMentions { ChannelActions channelActions, { required _NonMemberMentionScan scan, required ScaffoldMessengerState? messenger, + required VoidCallback ensureCurrent, }) async { final outcome = await _addMentionedNonMembers( channelActions, @@ -614,6 +621,7 @@ class _OutgoingMentions { agentPubkeys: scan.agentPubkeys, humanPubkeys: _invitedHumanPubkeys, canAddMembers: scan.canAddMembers, + ensureCurrent: ensureCurrent, ); demote(outcome.notAdded); if (outcome.errors.isNotEmpty) { diff --git a/mobile/test/features/channels/compose_bar_test.dart b/mobile/test/features/channels/compose_bar_test.dart index 23b19d9557..2e6c7cc35e 100644 --- a/mobile/test/features/channels/compose_bar_test.dart +++ b/mobile/test/features/channels/compose_bar_test.dart @@ -30,6 +30,8 @@ import 'package:buzz/shared/widgets/anchored_popover_menu.dart'; import 'package:buzz/shared/widgets/mobile_tab_footer_backdrop.dart'; import 'package:shared_preferences/shared_preferences.dart'; +part 'compose_bar_test/send_lifecycle_tests.dart'; + final _pngBytes = Uint8List.fromList([ 0x89, 0x50, @@ -174,6 +176,7 @@ Widget _buildComposeBar({ required ComposeBarOnSend onSend, List members = const [], Future>? membersFuture, + Future> Function()? membersLoader, List relayAgents = const [], List channels = const [], List cachedMembers = const [], @@ -190,6 +193,7 @@ Widget _buildComposeBar({ ValueChanged? onFocusRestorerChanged, AppLifecycleNotifier Function()? appLifecycle, String composeBarKey = 'compose-bar', + String? threadHeadId, VoiceNoteRecorder Function()? voiceNoteRecorderFactory, VoiceNotePlayerController Function()? voiceNotePlayerFactory, }) { @@ -207,9 +211,10 @@ Widget _buildComposeBar({ ), photoLibraryProvider.overrideWithValue(photoLibrary), currentPubkeyProvider.overrideWith((ref) => currentPubkey), - channelMembersProvider( - 'channel-1', - ).overrideWith((ref) => membersFuture ?? Future.value(members)), + channelMembersProvider('channel-1').overrideWith( + (ref) => + membersLoader?.call() ?? membersFuture ?? Future.value(members), + ), agentDirectoryProvider.overrideWith((ref) async => relayAgents), agentOwnersProvider.overrideWith((ref) async => const {}), relayClientProvider.overrideWithValue( @@ -253,6 +258,7 @@ Widget _buildComposeBar({ final composeBar = ComposeBar( key: ValueKey(composeBarKey), channelId: 'channel-1', + threadHeadId: threadHeadId, focusNode: focusNode, onFocusRestorerChanged: onFocusRestorerChanged, onFocusRequested: onFocusRequested, @@ -641,6 +647,7 @@ class _FakeChannelsNotifier extends ChannelsNotifier { } void main() { + sendLifecycleTests(); TestWidgetsFlutterBinding.ensureInitialized(); setUp(() async { @@ -3279,6 +3286,10 @@ void main() { publishedEvents.where((event) => event['kind'] == 9000), isEmpty, ); + expect( + tester.widget(find.byType(TextField)).controller!.text, + 'hello @Helper Bot', + ); }, ); diff --git a/mobile/test/features/channels/compose_bar_test/send_lifecycle_tests.dart b/mobile/test/features/channels/compose_bar_test/send_lifecycle_tests.dart new file mode 100644 index 0000000000..b39112cc4a --- /dev/null +++ b/mobile/test/features/channels/compose_bar_test/send_lifecycle_tests.dart @@ -0,0 +1,152 @@ +part of '../compose_bar_test.dart'; + +void sendLifecycleTests() { + for (final action in [ + 'double send', + 'edit', + 'revisit', + 'unmount', + 'failure', + ]) { + testWidgets('send preparation fences $action', (tester) async { + final gate = Completer>(); + final members = [ + ChannelMember( + pubkey: 'a' * 64, + displayName: 'Alice', + role: 'member', + joinedAt: DateTime(2025), + ), + ]; + var pending = false; + var sent = 0; + final service = _testUploadService(nostr.Keys.generate().nsec); + Widget build({String? thread}) => _buildComposeBar( + uploadService: service, + channels: [_makeCurrentChannel()], + membersLoader: () => pending ? gate.future : Future.value(members), + threadHeadId: thread, + onSend: (_, _, {mediaTags = const []}) async { + sent++; + }, + ); + await tester.pumpWidget(build()); + await _expandComposer(tester); + await tester.enterText(find.byType(TextField), '@ali'); + await tester.pumpAndSettle(); + await tester.tap(find.text('Alice')); + await tester.pumpAndSettle(); + final container = ProviderScope.containerOf( + tester.element(find.byType(ComposeBar)), + ); + pending = true; + container.invalidate(channelMembersProvider('channel-1')); + await tester.pump(); + // Invoke twice without a frame: an asynchronous latch is too late. + final button = tester.widget( + find + .ancestor( + of: find.byIcon(LucideIcons.arrowUp), + matching: find.byType(GestureDetector), + ) + .first, + ); + button.onTap!(); + if (action == 'double send') button.onTap!(); + await tester.pump(); + if (action == 'edit') { + await tester.enterText(find.byType(TextField), 'new intent'); + } + if (action == 'revisit') { + await tester.pumpWidget(build(thread: 'other-thread')); + await tester.pumpWidget(build()); + } + if (action == 'unmount') await tester.pumpWidget(const SizedBox()); + if (action == 'failure') { + gate.completeError(Exception('membership unavailable')); + } else { + gate.complete(members); + } + await tester.pumpAndSettle(); + await tester.pump(const Duration(milliseconds: 300)); + expect(sent, action == 'double send' ? 1 : 0); + if (action == 'edit') { + expect( + tester.widget(find.byType(TextField)).controller!.text, + 'new intent', + ); + } + if (action == 'failure') { + expect(find.textContaining('membership unavailable'), findsOneWidget); + expect( + tester.widget(find.byType(TextField)).controller!.text, + '@Alice ', + ); + } + }); + } + testWidgets('a revisited source cannot finish an old membership batch', ( + tester, + ) async { + final signer = nostr.Keys.generate(); + final gate = Completer(); + final events = >[]; + var sends = 0; + final service = _testUploadService(signer.nsec); + Widget build({String? thread}) => _buildComposeBar( + uploadService: service, + currentPubkey: signer.public, + relayAgents: [ + _testAgent('b' * 64), + AgentDirectoryEntry( + pubkey: 'c' * 64, + displayName: 'Other Bot', + respondTo: 'anyone', + channelIds: const ['shared-channel'], + ), + ], + channels: [_makeCurrentChannel(), _makeSharedMemberChannel()], + threadHeadId: thread, + onSend: (_, _, {mediaTags = const []}) async { + sends++; + }, + ); + await tester.pumpWidget(build()); + final container = ProviderScope.containerOf( + tester.element(find.byType(ComposeBar)), + ); + final session = container.read(relaySessionProvider.notifier); + session.debugAttachSocketForTest( + _RecordingRelaySocket( + events, + session.debugHandleSocketMessageForTest, + beforeAcknowledged: (event) async { + if (event['kind'] == 9000) await gate.future; + }, + ), + ); + await _expandComposer(tester); + await tester.enterText(find.byType(TextField), '@hel'); + await tester.pumpAndSettle(); + await tester.tap(find.text('Helper Bot')); + await tester.pumpAndSettle(); + await tester.enterText(find.byType(TextField), '@Helper Bot @oth'); + await tester.pumpAndSettle(); + await tester.tap(find.text('Other Bot')); + await tester.pumpAndSettle(); + await tester.tap(find.byIcon(LucideIcons.arrowUp)); + await tester.pump(); + expect(events.where((e) => e['kind'] == 9000), hasLength(1)); + await tester.pumpWidget(build(thread: 'other')); + await tester.pumpWidget(build()); + gate.complete(); + await tester.pumpAndSettle(); + await tester.pump(const Duration(milliseconds: 300)); + expect(sends, 0); + expect(events.where((e) => e['kind'] == 9000), hasLength(1)); + expect( + tester.widget(find.byType(TextField)).controller!.text, + '@Helper Bot @Other Bot ', + ); + }); +}