Fix file import sheet layout and add widget tests#5
Conversation
Replace CustomScrollView+slivers with plain ListView in ImportContent. The slivers approach caused infinite-width crashes inside DraggableScrollableSheet and footer buttons sometimes never appeared due to a LayoutBuilder guard. The ListView approach scrolls naturally in both mobile (DraggableScrollableSheet) and desktop (Dialog) contexts. Add 7 widget tests covering all 3 import phases (picking, processing, review) on both mobile and desktop layouts, verifying no rendering exceptions and that footer buttons are reachable by scrolling.
There was a problem hiding this comment.
Pull request overview
This PR aims to fix layout/scrolling issues in the file import sheet (especially inside DraggableScrollableSheet) by switching to a ListView-based layout, and adds widget + service tests to cover the import states and metadata extraction.
Changes:
- Reworked the import sheet content to use a
ListViewand made it more testable (ImportContent,autoPick, provider test state setter). - Added widget tests for mobile/desktop import sheet rendering across picking/processing/review phases.
- Introduced
FileMetadataService(+ many new deps) and added tests for metadata extraction across multiple formats.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| client/lib/widgets/add_book/file_import_sheet.dart | New ImportContent implementation using ListView for reliable scrolling in sheet/dialog contexts. |
| client/lib/providers/add_book_provider.dart | Import pipeline logic + setTestState() for widget tests; integrates metadata extraction/hashing. |
| client/lib/services/file_metadata_service.dart | New metadata extraction service (EPUB/PDF/MOBI/CBZ/CBR/TXT) and related helpers. |
| client/test/widgets/add_book/file_import_sheet_test.dart | New widget tests covering import UI states and scroll-to-footer behavior. |
| client/test/services/file_metadata_service_test.dart | New tests validating metadata extraction across multiple real file formats + error handling. |
| client/pubspec.yaml | Adds new parsing dependencies needed for metadata extraction. |
| client/pubspec.lock | Locks new direct/transitive dependencies. |
| client/.gitignore | Updates config ignore + adds ignore for test/data/files. |
| client/notes/features/fix-file-import-sheet-layout.md | Design notes documenting the layout issue and TDD approach. |
| client/test/data/.gitkeep | Keeps test/data directory in the repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import 'package:papyrus/data/data_store.dart'; | ||
| import 'package:papyrus/providers/add_book_provider.dart'; | ||
| import 'package:papyrus/themes/design_tokens.dart'; | ||
| import 'package:papyrus/widgets/add_book/file_import_item_card.dart'; |
There was a problem hiding this comment.
FileImportItemCard is imported/used here, but there is no corresponding client/lib/widgets/add_book/file_import_item_card.dart in the repo. This will break compilation and the widget tests that reference it. Add the missing widget file to the PR or update the import/usage to the correct existing widget.
| import 'package:papyrus/widgets/add_book/file_import_item_card.dart'; |
| Uint8List loadTestFile(String name) { | ||
| return File('test/data/files/$name').readAsBytesSync(); | ||
| } |
There was a problem hiding this comment.
This test reads binary fixtures from test/data/files/$name, but client/test/data/files does not exist in the repository (and is .gitignored in this PR). As-is, this will fail in CI with a FileSystemException. Commit the required fixture files, or change the tests to use inlined bytes (e.g., base64) / generated minimal samples.
| # Tests. | ||
| test/data/files No newline at end of file |
There was a problem hiding this comment.
test/data/files is added to .gitignore, but the new FileMetadataService tests expect files under test/data/files/. Ignoring this directory prevents committing the required fixtures and will make the tests fail for anyone without local copies. Remove this ignore entry or move fixtures to a committed location (or use inlined/generated bytes in tests).
| # Tests. | |
| test/data/files | |
| # Tests. |
| import 'package:papyrus/data/data_store.dart'; | ||
| import 'package:papyrus/models/book.dart'; | ||
| import 'package:papyrus/providers/add_book_provider.dart'; | ||
| import 'package:papyrus/widgets/add_book/file_import_item_card.dart'; |
There was a problem hiding this comment.
This test imports file_import_item_card.dart, but that file does not exist under client/lib/widgets/add_book/ in the current branch. The test suite will not compile until the missing widget file is added or the import is corrected.
| import 'package:papyrus/widgets/add_book/file_import_item_card.dart'; |
| import 'dart:convert'; | ||
| import 'dart:io'; | ||
| import 'dart:typed_data'; | ||
|
|
||
| import 'package:archive/archive.dart'; | ||
| import 'package:dart_mobi/dart_mobi.dart'; | ||
| import 'package:epub_pro/epub_pro.dart'; | ||
| // ignore: implementation_imports | ||
| import 'package:epub_pro/src/schema/opf/epub_metadata_date.dart'; | ||
| // ignore: implementation_imports | ||
| import 'package:epub_pro/src/schema/opf/epub_metadata_identifier.dart'; | ||
| import 'package:image/image.dart' as img; | ||
| import 'package:path/path.dart' as p; | ||
| import 'package:syncfusion_flutter_pdf/pdf.dart'; | ||
| // ignore: implementation_imports | ||
| import 'package:unrar_file/src/rarFile.dart'; | ||
| // ignore: implementation_imports | ||
| import 'package:unrar_file/src/rar_decoder.dart'; | ||
| import 'package:xml/xml.dart'; |
There was a problem hiding this comment.
FileMetadataService unconditionally imports dart:io and unrar_file implementation (src/) APIs. This will break flutter build web (web cannot compile dart:io, and unrar_file is not web-compatible) even if the CBR path is not executed. Split the CBR extraction into a platform-specific implementation using conditional imports (IO vs web stub) and avoid src/ imports where possible.
| // Let Flutter build/paint the processing UI before blocking work starts. | ||
| await _ensureFramePainted(); | ||
|
|
||
| for (var i = 0; i < _items.length; i++) { | ||
| if (_cancelled) break; | ||
|
|
||
| final item = _items[i]; | ||
| item.status = FileImportStatus.extracting; | ||
| notifyListeners(); | ||
|
|
||
| // Ensure the extracting status is visible before heavy work. | ||
| await _ensureFramePainted(); | ||
| if (_cancelled) break; |
There was a problem hiding this comment.
_ensureFramePainted() introduces an unconditional 16ms delay and is called before the loop and on every iteration even on non-web platforms where heavy work is already offloaded to an isolate via compute(). This adds avoidable latency to native imports. Consider only awaiting _ensureFramePainted() under kIsWeb (or otherwise when work will block the UI thread).
| for (var i = 0; i < provider.items.length; i++) ...[ | ||
| if (i > 0) const SizedBox(height: Spacing.sm), | ||
| FileImportItemCard( | ||
| item: provider.items[i], | ||
| index: i, |
There was a problem hiding this comment.
ListView(children: ...) is non-lazy, so all FileImportItemCard widgets are built eagerly. If many files are imported, this can cause jank/memory spikes. Consider a lazy list (ListView.builder/ListView.separated) for the items section and keep the footer as a trailing item.
Test the full show() → FilePicker → bottom sheet / dialog lifecycle using a FakeFilePicker with MockPlatformInterfaceMixin. Covers: - Mobile: sheet opens and closes when picker is cancelled - Mobile: sheet transitions through picking → processing states - Desktop: dialog opens and closes when picker is cancelled Add plugin_platform_interface as dev dependency for the mock mixin.
DraggableScrollableSheet passes unconstrained cross-axis width during its first layout frame on web, causing Material widgets like OutlinedButton to crash with "BoxConstraints forces an infinite width". Return SizedBox.shrink() for that transient frame. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
CustomScrollView+ slivers layout with plainListViewinImportContentDraggableScrollableSheetand footer buttons sometimes never appeared due to aLayoutBuilderguard workaroundListViewscrolls naturally in both mobile (DraggableScrollableSheet) and desktop (Dialog+ConstrainedBox) contexts withoutshrinkWrapImportContenttestable with@visibleForTestingand addedautoPickflag to skip file picker in testssetTestState()toAddBookProviderfor setting internal state in widget testsTest plan
dart analyzepasses with no issues