From 14886852c7d9087ecc05ea7203c4472ffd289885 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 8 Sep 2026 10:55:19 -0400 Subject: [PATCH 1/2] fix(superdeck): release capture resources and prune stale thumbnails Slide capture built a temporary element, render, and focus tree for every capture and never released it. Unmount that subtree and dispose the render objects, the pipeline owner, and the focus manager on success, on the settle limit, and on failure. Dispose the captured image even when PNG encoding fails. Thumbnail cleanup ran only inside thumbnail warmup, so a deck that lost slides or became empty kept obsolete handles. Prune the thumbnail cache from the slide collection itself. Guard overlapping slide transitions with an operation identifier, so an earlier delay cannot clear the state of a later transition. --- .../src/capture/slide_capture_service.dart | 99 ++++++++-- .../lib/src/deck/deck_presentation_state.dart | 53 ++++-- .../capture/slide_capture_teardown_test.dart | 177 ++++++++++++++++++ .../deck/deck_presentation_state_test.dart | 111 +++++++++++ 4 files changed, 407 insertions(+), 33 deletions(-) create mode 100644 packages/superdeck/test/src/capture/slide_capture_teardown_test.dart diff --git a/packages/superdeck/lib/src/capture/slide_capture_service.dart b/packages/superdeck/lib/src/capture/slide_capture_service.dart index 7c342f70..71fcd8d9 100644 --- a/packages/superdeck/lib/src/capture/slide_capture_service.dart +++ b/packages/superdeck/lib/src/capture/slide_capture_service.dart @@ -206,19 +206,33 @@ class SlideCaptureService { } Future _imageToUint8List(ui.Image image) async { - final byteData = await image.toByteData(format: ui.ImageByteFormat.png); - image.dispose(); - return byteData!.buffer.asUint8List(); + try { + final byteData = await image.toByteData(format: ui.ImageByteFormat.png); + + return byteData!.buffer.asUint8List(); + } finally { + image.dispose(); + } } /// Converts a Flutter widget to a [ui.Image] via an isolated render pipeline. /// /// Sets up a complete render context (theme, media query, material app), /// drives a bounded settle loop for async/delayed widgets, then rasterises. + /// Releases the temporary element, render, and focus resources on success, + /// on the settle limit, and on failure. Future _fromWidgetToImage( Widget widget, RenderConfig config, ) async { + RenderRepaintBoundary? repaintBoundary; + RenderPositionedBox? rootBox; + RenderView? renderView; + PipelineOwner? pipelineOwner; + FocusManager? focusManager; + BuildOwner? buildOwner; + RenderObjectToWidgetElement? rootElement; + try { final mixScope = MixScope.maybeOf(config.context); final readiness = SlideCaptureReadiness(); @@ -242,7 +256,11 @@ class SlideCaptureService { ), ); - final repaintBoundary = RenderRepaintBoundary(); + repaintBoundary = RenderRepaintBoundary(); + rootBox = RenderPositionedBox( + alignment: Alignment.center, + child: repaintBoundary, + ); final platformDispatcher = WidgetsBinding.instance.platformDispatcher; final view = @@ -251,12 +269,9 @@ class SlideCaptureService { config.targetSize ?? view.physicalSize / view.devicePixelRatio; final physicalSize = logicalSize * config.pixelRatio; - final renderView = RenderView( + renderView = RenderView( view: view, - child: RenderPositionedBox( - alignment: Alignment.center, - child: repaintBoundary, - ), + child: rootBox, configuration: ViewConfiguration( logicalConstraints: BoxConstraints.tight(logicalSize), physicalConstraints: BoxConstraints.tight(physicalSize), @@ -265,18 +280,17 @@ class SlideCaptureService { ); var isDirty = false; - final pipelineOwner = PipelineOwner( - onNeedVisualUpdate: () => isDirty = true, - ); - final buildOwner = BuildOwner( - focusManager: FocusManager(), + pipelineOwner = PipelineOwner(onNeedVisualUpdate: () => isDirty = true); + focusManager = FocusManager(); + buildOwner = BuildOwner( + focusManager: focusManager, onBuildScheduled: () => isDirty = true, ); pipelineOwner.rootNode = renderView; renderView.prepareInitialFrame(); - final rootElement = RenderObjectToWidgetAdapter( + rootElement = RenderObjectToWidgetAdapter( container: repaintBoundary, child: Directionality(textDirection: TextDirection.ltr, child: child), ).attachToRenderTree(buildOwner); @@ -331,6 +345,61 @@ class SlideCaptureService { } catch (e) { log('Error finalizing tree: $e'); rethrow; + } finally { + _releaseCaptureTree( + buildOwner: buildOwner, + rootElement: rootElement, + repaintBoundary: repaintBoundary, + rootBox: rootBox, + renderView: renderView, + pipelineOwner: pipelineOwner, + focusManager: focusManager, + ); + } + } + + /// Unmounts the temporary capture subtree and releases its owned resources. + /// + /// Rebuilding the root adapter without a child deactivates the captured + /// widgets, and [BuildOwner.finalizeTree] then unmounts them so their + /// [State.dispose] and render object disposal run. The render pipeline, + /// the render objects this service created, and the focus manager are + /// released afterwards. + void _releaseCaptureTree({ + required BuildOwner? buildOwner, + required RenderObjectToWidgetElement? rootElement, + required RenderRepaintBoundary? repaintBoundary, + required RenderPositionedBox? rootBox, + required RenderView? renderView, + required PipelineOwner? pipelineOwner, + required FocusManager? focusManager, + }) { + if (buildOwner != null && rootElement != null && repaintBoundary != null) { + try { + RenderObjectToWidgetAdapter( + container: repaintBoundary, + ).attachToRenderTree(buildOwner, rootElement); + buildOwner + ..buildScope(rootElement) + ..finalizeTree(); + } catch (e, stackTrace) { + log('Error unmounting capture tree: $e', stackTrace: stackTrace); + } + } + + if (pipelineOwner != null) { + pipelineOwner.rootNode = null; + pipelineOwner.dispose(); + } + + repaintBoundary?.dispose(); + rootBox?.dispose(); + renderView?.dispose(); + + if (focusManager != null) { + // Unmounting focus nodes schedules a focus update microtask. Disposing + // in a later microtask lets that update run against a live manager. + scheduleMicrotask(focusManager.dispose); } } } diff --git a/packages/superdeck/lib/src/deck/deck_presentation_state.dart b/packages/superdeck/lib/src/deck/deck_presentation_state.dart index 616717e0..ac320f28 100644 --- a/packages/superdeck/lib/src/deck/deck_presentation_state.dart +++ b/packages/superdeck/lib/src/deck/deck_presentation_state.dart @@ -19,6 +19,8 @@ final class DeckPresentationState { final _thumbnails = signal>({}); EffectCleanup? _indexClampEffect; + EffectCleanup? _thumbnailPruneEffect; + int _transitionOperation = 0; bool _disposed = false; late final GoRouter router = GoRouter( @@ -57,6 +59,11 @@ final class DeckPresentationState { _currentIndex.value = clamped; } }); + // Thumbnail cleanup follows the slide collection, not thumbnail warmup, + // so obsolete handles are released even when the deck becomes empty. + _thumbnailPruneEffect = effect(() { + _pruneThumbnails(_slideKeys(_slides.value)); + }); } ReadonlySignal get isMenuOpen => _isMenuOpen; @@ -96,10 +103,13 @@ final class DeckPresentationState { Future goToSlide(int index) async { if (_disposed || index < 0 || index >= _slides.value.length) return; + // Only the latest transition may clear the transitioning state, so an + // earlier delay cannot end a transition that started after it. + final operation = ++_transitionOperation; _isTransitioning.value = true; router.go('/slides/$index'); await Future.delayed(_transitionDuration); - if (_disposed) return; + if (_disposed || operation != _transitionOperation) return; _isTransitioning.value = false; } @@ -121,28 +131,14 @@ final class DeckPresentationState { bool force = false, }) { if (_disposed) return; - if (slides.isEmpty) return; - - final validKeys = slides.map((s) => s.key).toSet(); - final current = _thumbnails.value; - final staleKeys = current.keys - .where((k) => !validKeys.contains(k)) - .toList(growable: false); - final cache = staleKeys.isEmpty - ? current - : Map.from(current); - for (final key in staleKeys) { - cache.remove(key)?.dispose(); - } - if (staleKeys.isNotEmpty) { - _thumbnails.value = cache; - } + _pruneThumbnails(_slideKeys(slides)); + if (slides.isEmpty) return; _thumbnailService.generateThumbnails( slides: slides, context: context, - cache: cache, + cache: _thumbnails.peek(), onCacheUpdate: (updated) { if (_disposed) return; _thumbnails.value = updated; @@ -174,6 +170,7 @@ final class DeckPresentationState { void dispose() { _disposed = true; _indexClampEffect?.call(); + _thumbnailPruneEffect?.call(); router.routeInformationProvider.removeListener(_syncCurrentIndexFromRouter); router.dispose(); for (final thumbnail in _thumbnails.value.values) { @@ -190,6 +187,23 @@ final class DeckPresentationState { currentSlide.dispose(); } + /// Disposes and drops every thumbnail whose slide is no longer present. + void _pruneThumbnails(Set validKeys) { + if (_disposed) return; + + final current = _thumbnails.peek(); + final staleKeys = current.keys + .where((key) => !validKeys.contains(key)) + .toList(growable: false); + if (staleKeys.isEmpty) return; + + final cache = Map.from(current); + for (final key in staleKeys) { + cache.remove(key)?.dispose(); + } + _thumbnails.value = cache; + } + void _syncCurrentIndexFromRouter() { if (_disposed) return; final path = router.routeInformationProvider.value.uri.path; @@ -205,6 +219,9 @@ final class DeckPresentationState { } } + static Set _slideKeys(List slides) => + slides.map((slide) => slide.key).toSet(); + static int _clampIndex(int index, int totalSlides) { final maxIndex = totalSlides > 0 ? totalSlides - 1 : 0; return index.clamp(0, maxIndex); diff --git a/packages/superdeck/test/src/capture/slide_capture_teardown_test.dart b/packages/superdeck/test/src/capture/slide_capture_teardown_test.dart new file mode 100644 index 00000000..11fe88f8 --- /dev/null +++ b/packages/superdeck/test/src/capture/slide_capture_teardown_test.dart @@ -0,0 +1,177 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:superdeck/superdeck.dart'; +import 'package:superdeck_core/superdeck_core.dart'; + +/// Records the mount lifecycle of one widget inside a capture subtree. +class _LifecycleRecord { + var mounted = false; + var disposed = false; +} + +class _LifecycleWidget extends StatefulWidget { + final _LifecycleRecord record; + final bool trackReadiness; + final bool failBuild; + + const _LifecycleWidget({ + required this.record, + this.trackReadiness = false, + this.failBuild = false, + }); + + @override + State<_LifecycleWidget> createState() => _LifecycleWidgetState(); +} + +class _LifecycleWidgetState extends State<_LifecycleWidget> { + SlideCaptureReadinessHandle? _readiness; + + @override + void initState() { + super.initState(); + widget.record.mounted = true; + } + + @override + void didChangeDependencies() { + super.didChangeDependencies(); + if (widget.trackReadiness) { + _readiness ??= SlideCaptureReadiness.track( + context, + label: 'teardown-test-widget', + ); + } + } + + @override + void dispose() { + widget.record.disposed = true; + super.dispose(); + } + + @override + Widget build(BuildContext context) { + if (widget.failBuild) { + throw StateError('captured widget failed to build'); + } + + return const SizedBox.expand(); + } +} + +SlideConfiguration _lifecycleSlide( + _LifecycleRecord record, { + bool trackReadiness = false, + bool failBuild = false, +}) { + return SlideConfiguration( + slideIndex: 0, + style: SlideStyler(), + slide: Slide( + key: 'lifecycle', + sections: [ + SectionBlock([WidgetBlock(name: 'lifecycle', args: const {})]), + ], + ), + widgets: { + 'lifecycle': (_) => _LifecycleWidget( + record: record, + trackReadiness: trackReadiness, + failBuild: failBuild, + ), + }, + thumbnailKey: 'thumbnail_lifecycle.png', + ); +} + +Future _pumpContext(WidgetTester tester) async { + final key = GlobalKey(); + await tester.pumpWidget(MaterialApp(home: SizedBox(key: key))); + + return key.currentContext!; +} + +void main() { + group('Slide capture teardown', () { + testWidgets('unmounts the capture subtree after a successful capture', ( + tester, + ) async { + final context = await _pumpContext(tester); + final record = _LifecycleRecord(); + + await tester.runAsync(() async { + final bytes = await SlideCaptureService().capture( + slide: _lifecycleSlide(record), + context: context, + ); + + expect(bytes, isNotEmpty); + }); + + expect(record.mounted, isTrue); + expect(record.disposed, isTrue); + }); + + testWidgets('unmounts the capture subtree after the settle limit', ( + tester, + ) async { + final context = await _pumpContext(tester); + final record = _LifecycleRecord(); + + await tester.runAsync(() async { + final bytes = await SlideCaptureService().capture( + slide: _lifecycleSlide(record, trackReadiness: true), + context: context, + ); + + expect(bytes, isNotEmpty); + }); + + expect(record.mounted, isTrue); + expect(record.disposed, isTrue); + }); + + testWidgets('unmounts the capture subtree when a captured widget fails', ( + tester, + ) async { + final context = await _pumpContext(tester); + final record = _LifecycleRecord(); + + await tester.runAsync(() async { + final bytes = await SlideCaptureService().capture( + slide: _lifecycleSlide(record, failBuild: true), + context: context, + ); + + expect(bytes, isNotEmpty); + }); + + expect(tester.takeException(), isStateError); + expect(record.mounted, isTrue); + expect(record.disposed, isTrue); + }); + + testWidgets('releases the subtree of every capture in a sequence', ( + tester, + ) async { + final context = await _pumpContext(tester); + final records = List.generate(3, (_) => _LifecycleRecord()); + final service = SlideCaptureService(); + + await tester.runAsync(() async { + for (final record in records) { + final bytes = await service.capture( + slide: _lifecycleSlide(record), + context: context, + ); + + expect(bytes, isNotEmpty); + } + }); + + expect(records.every((record) => record.mounted), isTrue); + expect(records.every((record) => record.disposed), isTrue); + }); + }); +} diff --git a/packages/superdeck/test/src/deck/deck_presentation_state_test.dart b/packages/superdeck/test/src/deck/deck_presentation_state_test.dart index d79b407a..df431dd0 100644 --- a/packages/superdeck/test/src/deck/deck_presentation_state_test.dart +++ b/packages/superdeck/test/src/deck/deck_presentation_state_test.dart @@ -114,6 +114,117 @@ void main() { expect(service.receivedCacheKeys.last, equals({'slide-0'})); }); + testWidgets('slide removal disposes stale thumbnails without warmup', ( + tester, + ) async { + final slides = signal>(createTestSlides(2)); + addTearDown(slides.dispose); + + final service = _RecordingThumbnailService(); + final state = DeckPresentationState( + thumbnailService: service, + slides: slides, + transitionDuration: Duration.zero, + ); + addTearDown(state.dispose); + + final context = await _pumpContext(tester); + state.generateThumbnails(context, slides.value); + final staleThumbnail = service.trackedThumbnails['slide-1']!; + final callsBeforeRemoval = service.callCount; + + slides.value = [slides.value.first]; + + expect(service.callCount, callsBeforeRemoval); + expect(staleThumbnail.disposed, isTrue); + expect(state.getThumbnail('slide-0'), isNotNull); + expect(state.getThumbnail('slide-1'), isNull); + }); + + testWidgets('an empty slide collection disposes every thumbnail', ( + tester, + ) async { + final slides = signal>(createTestSlides(2)); + addTearDown(slides.dispose); + + final service = _RecordingThumbnailService(); + final state = DeckPresentationState( + thumbnailService: service, + slides: slides, + transitionDuration: Duration.zero, + ); + addTearDown(state.dispose); + + final context = await _pumpContext(tester); + state.generateThumbnails(context, slides.value); + final slide0 = service.trackedThumbnails['slide-0']!; + final slide1 = service.trackedThumbnails['slide-1']!; + + slides.value = const []; + + expect(slide0.disposed, isTrue); + expect(slide1.disposed, isTrue); + expect(state.getThumbnail('slide-0'), isNull); + expect(state.getThumbnail('slide-1'), isNull); + }); + + testWidgets('an empty warmup list disposes every thumbnail', ( + tester, + ) async { + final slides = signal>(createTestSlides(2)); + addTearDown(slides.dispose); + + final service = _RecordingThumbnailService(); + final state = DeckPresentationState( + thumbnailService: service, + slides: slides, + transitionDuration: Duration.zero, + ); + addTearDown(state.dispose); + + final context = await _pumpContext(tester); + state.generateThumbnails(context, slides.value); + final slide0 = service.trackedThumbnails['slide-0']!; + final slide1 = service.trackedThumbnails['slide-1']!; + + state.generateThumbnails(context, const []); + + expect(slide0.disposed, isTrue); + expect(slide1.disposed, isTrue); + expect(state.getThumbnail('slide-0'), isNull); + expect(state.getThumbnail('slide-1'), isNull); + }); + + testWidgets('a superseded transition keeps the latest transition open', ( + tester, + ) async { + const transitionDuration = Duration(milliseconds: 300); + final slides = signal>(createTestSlides(3)); + addTearDown(slides.dispose); + + final service = _RecordingThumbnailService(); + final state = DeckPresentationState( + thumbnailService: service, + slides: slides, + transitionDuration: transitionDuration, + ); + addTearDown(state.dispose); + + final first = state.goToSlide(1); + await tester.pump(const Duration(milliseconds: 200)); + final second = state.goToSlide(2); + + // The first transition's delay elapses while the second one is running. + await tester.pump(const Duration(milliseconds: 150)); + expect(state.isTransitioning.value, isTrue); + + await tester.pump(const Duration(milliseconds: 200)); + await first; + await second; + + expect(state.isTransitioning.value, isFalse); + }); + testWidgets('deleteAllThumbnails disposes cache and clears getThumbnail', ( tester, ) async { From 3d78cead267268bf31aa8502cee7509815a77efe Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Tue, 8 Sep 2026 14:45:37 -0400 Subject: [PATCH 2/2] ci: validate every branch in pull request stacks --- .github/workflows/test.yml | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9fc0e906..16a54880 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -5,7 +5,6 @@ on: push: branches: [main] pull_request: - branches: [main] concurrency: group: test-${{ github.ref }} @@ -17,13 +16,13 @@ jobs: name: Test timeout-minutes: 12 steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Install FVM shell: bash run: | curl -fsSL https://fvm.app/install.sh | bash - echo "/home/runner/fvm/bin" >> $GITHUB_PATH + echo "/home/runner/fvm/bin" >> "$GITHUB_PATH" - uses: kuhnroyal/flutter-fvm-config-action@v2 id: fvm-config-action @@ -71,7 +70,7 @@ jobs: name: Integration Tests timeout-minutes: 25 steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Install Linux Desktop Dependencies timeout-minutes: 8 @@ -87,7 +86,7 @@ jobs: shell: bash run: | curl -fsSL https://fvm.app/install.sh | bash - echo "/home/runner/fvm/bin" >> $GITHUB_PATH + echo "/home/runner/fvm/bin" >> "$GITHUB_PATH" - uses: kuhnroyal/flutter-fvm-config-action@v2 id: fvm-config-action @@ -137,7 +136,7 @@ jobs: shell: bash run: | curl -fsSL https://fvm.app/install.sh | bash - echo "/home/runner/fvm/bin" >> $GITHUB_PATH + echo "/home/runner/fvm/bin" >> "$GITHUB_PATH" - uses: kuhnroyal/flutter-fvm-config-action@v2 id: fvm-config-action