From 5767e061cd8f25cba2fab4b660927b8b7ccf2a44 Mon Sep 17 00:00:00 2001 From: davidwei Date: Tue, 16 Jun 2026 20:32:47 +1200 Subject: [PATCH 1/2] fix: remove leaked NoteModel listener in NoteEditController The anonymous listener registered in initialize() via addPostFrameCallback could not be removed, so any notifyListeners() call after the widget was disposed (e.g. in-flight image upload completing post-navigation) would write to an already-disposed TextEditingController. Hoists the lambda to a named _syncControllerFromModel method, stores the NoteModel reference, and removes the listener in dispose(). Guards the addPostFrameCallback body with _disposed so that dispose-before-frame races also produce a no-op. Adds two regression tests covering the post-dispose and race paths. Closes #37 Co-Authored-By: Claude Sonnet 4.6 --- .../controllers/note_edit_controller.dart | 24 +++++++--- .../note_edit_controller_test.dart | 46 +++++++++++++++++++ 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/lib/screens/components/controllers/note_edit_controller.dart b/lib/screens/components/controllers/note_edit_controller.dart index 2cb439f..622eb1f 100644 --- a/lib/screens/components/controllers/note_edit_controller.dart +++ b/lib/screens/components/controllers/note_edit_controller.dart @@ -15,15 +15,29 @@ class NoteEditController { final HtmlToMarkdownConverter htmlToMarkdownConverter; TextEditingController textController = TextEditingController(); + NoteModel? _noteModel; + bool _disposed = false; + NoteEditController({ required this.imageService, required this.clipboardService, required this.htmlToMarkdownConverter, }); + void _syncControllerFromModel() { + if (_disposed) return; + final noteModel = _noteModel; + if (noteModel == null) return; + if (noteModel.content != textController.text) { + textController.text = noteModel.content; + } + } + void initialize(NoteModel noteModel, Note? note, BuildContext context) { // Delay the update to avoid triggering a rebuild during the build phase WidgetsBinding.instance.addPostFrameCallback((_) { + if (_disposed) return; + if (noteModel.initialContent.isNotEmpty && note == null) { noteModel.content = noteModel.initialContent; } else if (note != null) { @@ -33,12 +47,8 @@ class NoteEditController { // Set the initial text in the controller textController.text = noteModel.content; - // Add listener to update controller.text when noteModel.content changes - noteModel.addListener(() { - if (noteModel.content != textController.text) { - textController.text = noteModel.content; - } - }); + _noteModel = noteModel; + noteModel.addListener(_syncControllerFromModel); // Request focus noteModel.requestFocus(); @@ -196,6 +206,8 @@ class NoteEditController { } void dispose() { + _disposed = true; + _noteModel?.removeListener(_syncControllerFromModel); textController.dispose(); } } diff --git a/test/controllers/note_edit_controller_test.dart b/test/controllers/note_edit_controller_test.dart index f18bcf2..1e4c1bb 100644 --- a/test/controllers/note_edit_controller_test.dart +++ b/test/controllers/note_edit_controller_test.dart @@ -314,4 +314,50 @@ void main() { expect(noteModel.isPasting, isFalse); }); }); + + group('NoteEditController listener lifecycle', () { + late NoteEditController controller; + + setUp(() { + controller = NoteEditController( + imageService: FakeImageService(), + clipboardService: FakeClipboardService(), + htmlToMarkdownConverter: HtmlToMarkdownConverter(), + ); + }); + + testWidgets('post-dispose notifyListeners does not throw', (tester) async { + final noteModel = NoteModel(); + await tester.pumpWidget(MaterialApp( + home: Builder(builder: (ctx) { + controller.initialize(noteModel, null, ctx); + return const SizedBox(); + }), + )); + await tester.pump(); // allow addPostFrameCallback to fire + + controller.dispose(); + + // Simulates in-flight upload/paste completing after navigation away + expect(() => noteModel.setUploading(true), returnsNormally); + expect(() => noteModel.setUploading(false), returnsNormally); + expect(() => noteModel.setPasting(true), returnsNormally); + expect(() => noteModel.setPasting(false), returnsNormally); + }); + + testWidgets('dispose before frame callback does not throw', (tester) async { + final noteModel = NoteModel(); + await tester.pumpWidget(MaterialApp( + home: Builder(builder: (ctx) { + controller.initialize(noteModel, null, ctx); + // dispose immediately, before the frame callback fires + controller.dispose(); + return const SizedBox(); + }), + )); + await tester.pump(); // callback fires but should be a no-op + + expect(() => noteModel.setUploading(false), returnsNormally); + }); + }); } From b48181e1e93669eda2044d4401b18268712b956e Mon Sep 17 00:00:00 2001 From: davidwei Date: Tue, 16 Jun 2026 20:43:20 +1200 Subject: [PATCH 2/2] fix: address review findings in NoteEditController listener fix - Add assert in initialize() to catch double-initialization in debug builds - Null _noteModel in dispose() after removeListener to release the reference - Fix weak test: setUploading(false) is a no-op when already false; call setUploading(true) first so notifyListeners() actually fires in the dispose-before-frame-callback test Co-Authored-By: Claude Sonnet 4.6 --- lib/screens/components/controllers/note_edit_controller.dart | 2 ++ test/controllers/note_edit_controller_test.dart | 3 +++ 2 files changed, 5 insertions(+) diff --git a/lib/screens/components/controllers/note_edit_controller.dart b/lib/screens/components/controllers/note_edit_controller.dart index 622eb1f..64cc751 100644 --- a/lib/screens/components/controllers/note_edit_controller.dart +++ b/lib/screens/components/controllers/note_edit_controller.dart @@ -34,6 +34,7 @@ class NoteEditController { } void initialize(NoteModel noteModel, Note? note, BuildContext context) { + assert(_noteModel == null, 'NoteEditController.initialize() must not be called more than once'); // Delay the update to avoid triggering a rebuild during the build phase WidgetsBinding.instance.addPostFrameCallback((_) { if (_disposed) return; @@ -208,6 +209,7 @@ class NoteEditController { void dispose() { _disposed = true; _noteModel?.removeListener(_syncControllerFromModel); + _noteModel = null; textController.dispose(); } } diff --git a/test/controllers/note_edit_controller_test.dart b/test/controllers/note_edit_controller_test.dart index 1e4c1bb..47d0bfa 100644 --- a/test/controllers/note_edit_controller_test.dart +++ b/test/controllers/note_edit_controller_test.dart @@ -357,6 +357,9 @@ void main() { )); await tester.pump(); // callback fires but should be a no-op + // setUploading(true) changes state and fires notifyListeners — the + // listener must not touch the disposed textController. + expect(() => noteModel.setUploading(true), returnsNormally); expect(() => noteModel.setUploading(false), returnsNormally); }); });