Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pdf_book/pdf_book_screen.dart (1)
231-234: Avoid allocating a FocusNode in build; it leaks.Passing a new
FocusNode()every build means it’s never disposed. LetFocusmanage its own node or hoist/dispose it in State.Apply this diff (simple, safe option):
- focusNode: FocusNode(), autofocus: !Platform.isAndroid,Or hoist a
late final FocusNode _rootFocus;in State, init ininitState(), and dispose indispose().
🧹 Nitpick comments (3)
lib/pdf_book/pdf_book_screen.dart (3)
352-357: Tooltip says “print” but code shares; switch to real print or rename UI.Use
Printing.layoutPdffor printing.Apply this diff:
- IconButton( - icon: const Icon(Icons.print), - tooltip: 'הדפס', - onPressed: () async { - await Printing.sharePdf( - bytes: File(widget.tab.book.path).readAsBytesSync(), - ); - }, - ), + IconButton( + icon: const Icon(Icons.print), + tooltip: 'הדפס', + onPressed: () async { + final bytes = await File(widget.tab.book.path).readAsBytes(); + await Printing.layoutPdf(onLayout: (_) async => bytes); + }, + ),Alternatively, keep sharing but change icon/tooltip to match.
134-140: Initialize sidebar width from current SettingsBloc state to avoid first-frame flicker.Reading from
Settingsthen immediately overriding from the bloc can cause a visible jump.Apply this diff:
- _sidebarWidth = ValueNotifier<double>( - Settings.getValue<double>('key-sidebar-width', defaultValue: 300)!); + _sidebarWidth = ValueNotifier<double>( + context.read<SettingsBloc>().state.sidebarWidth);
59-59: Gate debug logging.Wrap debug prints or switch to
debugPrintto avoid noisy logs in release.Apply this diff:
- print('DEBUG: Triggering search by simulating user input for "$query"'); + debugPrint('Triggering initial search for "$query"');- print('DEBUG: אתחול PDF טאב - דף התחלתי: ${widget.tab.pageNumber}'); + debugPrint('Init PDF tab - initial page: ${widget.tab.pageNumber}');And add once at imports:
import 'package:flutter/foundation.dart';Also applies to: 132-132
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/pdf_book/pdf_book_screen.dart(18 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_linux
- GitHub Check: build_macos
- GitHub Check: build_android
- GitHub Check: build_windows
🔇 Additional comments (1)
lib/pdf_book/pdf_book_screen.dart (1)
101-116: LGTM: clean search-session reconciliation.The persisted-index restoration guarded by
searchSessionis correct and avoids jumping on incremental updates.
| } | ||
|
|
||
| late TabController _tabController; |
There was a problem hiding this comment.
Remove unused TabController or dispose it to avoid a ticker leak.
_tabController is created but never used or disposed, which will leak a Ticker.
Apply this diff to remove it:
- late TabController _tabController;- _tabController = TabController(
- length: 3,
- vsync: this,
- initialIndex: widget.tab.searchText.isNotEmpty ? 1 : 0,
- );If you intend to keep it, ensure you use it and call _tabController.dispose() in dispose().
Also applies to: 144-148
🤖 Prompt for AI Agents
In lib/pdf_book/pdf_book_screen.dart around lines 85 and also covering 144-148,
the TabController field is declared but never used or disposed which causes a
Ticker leak; either remove the unused `late TabController _tabController;`
declaration and any related unused code, or properly initialize it (e.g., in
initState with vsync: this), use it for TabBar/TabBarView, and call
`_tabController.dispose()` inside the widget's `dispose()` method to release the
ticker.
| onHorizontalDragUpdate: (details) { | ||
| final newWidth = | ||
| (_sidebarWidth.value - details.delta.dx) | ||
| .clamp(200.0, 600.0); | ||
| _sidebarWidth.value = newWidth; | ||
| }, |
There was a problem hiding this comment.
Fix drag direction: dragging right should widen the sidebar, not shrink it.
Use +details.delta.dx instead of subtraction; current behavior inverts resize.
Apply this diff:
- final newWidth =
- (_sidebarWidth.value - details.delta.dx)
+ final newWidth =
+ (_sidebarWidth.value + details.delta.dx)
.clamp(200.0, 600.0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onHorizontalDragUpdate: (details) { | |
| final newWidth = | |
| (_sidebarWidth.value - details.delta.dx) | |
| .clamp(200.0, 600.0); | |
| _sidebarWidth.value = newWidth; | |
| }, | |
| onHorizontalDragUpdate: (details) { | |
| final newWidth = | |
| (_sidebarWidth.value + details.delta.dx) | |
| .clamp(200.0, 600.0); | |
| _sidebarWidth.value = newWidth; | |
| }, |
🤖 Prompt for AI Agents
In lib/pdf_book/pdf_book_screen.dart around lines 372 to 377, the horizontal
drag update is inverting the resize direction (dragging right currently shrinks
the sidebar). Change the width calculation to add the drag delta: compute
newWidth as (_sidebarWidth.value + details.delta.dx).clamp(200.0, 600.0) and
assign that to _sidebarWidth.value so dragging right widens and dragging left
narrows within bounds.
| ? bytesDownloaded / totalBytes | ||
| : null, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against division by zero in progress indicator.
If totalBytes == 0, the value becomes Infinity.
Apply this diff:
- value: totalBytes != null
- ? bytesDownloaded / totalBytes
- : null,
+ value: (totalBytes ?? 0) > 0
+ ? bytesDownloaded / totalBytes!
+ : null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ? bytesDownloaded / totalBytes | |
| : null, | |
| value: (totalBytes ?? 0) > 0 | |
| ? bytesDownloaded / totalBytes! | |
| : null, |
🤖 Prompt for AI Agents
In lib/pdf_book/pdf_book_screen.dart around lines 475 to 476, the progress value
currently computes bytesDownloaded / totalBytes which will produce Infinity when
totalBytes == 0; modify the ternary condition to also check totalBytes > 0
(e.g., use totalBytes != null && totalBytes > 0 ? bytesDownloaded / totalBytes :
null) so division only occurs when totalBytes is a positive nonzero value.
| return MouseRegion( | ||
| cursor: SystemMouseCursors.click, | ||
| child: GestureDetector( | ||
| onTap: () { | ||
| controller.animateTo(index); | ||
| }, | ||
| child: Material( | ||
| color: Colors.transparent, | ||
| child: InkWell( | ||
| onTap: () { | ||
| controller.animateTo(index); | ||
| }, | ||
| child: Container( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate tap handlers on the custom tab cause double navigation.
Both GestureDetector.onTap and InkWell.onTap call animateTo. Keep only one (prefer InkWell for ripple).
Apply this diff:
- child: GestureDetector(
- onTap: () {
- controller.animateTo(index);
- },
- child: Material(
- color: Colors.transparent,
- child: InkWell(
- onTap: () {
- controller.animateTo(index);
- },
- child: Container(
+ child: Material(
+ color: Colors.transparent,
+ child: InkWell(
+ onTap: () {
+ controller.animateTo(index);
+ },
+ child: Container(
...
- ),
- ),
- ),
- ),
+ ),
+ ),
+ ),Also applies to: 779-781
🤖 Prompt for AI Agents
In lib/pdf_book/pdf_book_screen.dart around lines 745-757 (and similarly at
lines ~779-781), there are duplicate tap handlers: both GestureDetector.onTap
and InkWell.onTap call controller.animateTo, causing double navigation; remove
the outer GestureDetector.onTap (or the inner InkWell.onTap) and keep only the
InkWell.onTap to preserve the ripple effect, ensuring only a single call to
controller.animateTo(index) remains and update surrounding code to avoid an
unused/empty onTap on GestureDetector if you remove the InkWell instead.
| future: DataRepository.instance.library | ||
| .then((library) => library.findBookByTitle(book.title, TextBook)), | ||
| builder: (context, snapshot) => snapshot.hasData |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize FutureBuilder: avoid recreating the Future on every build.
library.then(...) allocates a new Future each build and can refetch. Cache it in State.
Apply this diff:
- future: DataRepository.instance.library
- .then((library) => library.findBookByTitle(book.title, TextBook)),
+ future: _textBookFuture,Add these outside this hunk:
// In _PdfBookScreenState:
late final Future<dynamic> _textBookFuture;
@override
void initState() {
super.initState();
_textBookFuture = DataRepository.instance.library
.then((library) => library.findBookByTitle(widget.tab.book.title, TextBook));
// ...rest of initState
}🤖 Prompt for AI Agents
In lib/pdf_book/pdf_book_screen.dart around lines 790 to 792, the FutureBuilder
currently calls DataRepository.instance.library.then(...) directly in build
which recreates the Future every build; move that Future creation into State to
stabilize it by adding a late final Future field (e.g. _textBookFuture) to
_PdfBookScreenState and initializing it inside initState using the widget's book
title (widget.tab.book.title) to call
DataRepository.instance.library.then((library) => library.findBookByTitle(...,
TextBook)); then update the FutureBuilder to use this cached _textBookFuture
instead of the inline library.then(...) expression.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
lib/navigation/main_window_screen.dart (1)
174-181: Consider extracting focus request logic to reduce code duplication.The focus request logic is repeated three times with identical patterns. Consider extracting this into a helper method.
Add a helper method to reduce duplication:
+ void _requestSearchFocus(BuildContext context, Screen screen) { + final focusRepo = context.read<FocusRepository>(); + if (screen == Screen.library) { + focusRepo.requestLibrarySearchFocus(selectAll: true); + } else if (screen == Screen.find) { + focusRepo.requestFindRefSearchFocus(selectAll: true); + } + }Then simplify the repeated code blocks:
- if (state.currentScreen == Screen.library) { - context - .read<FocusRepository>() - .requestLibrarySearchFocus(selectAll: true); - } else if (state.currentScreen == Screen.find) { - context - .read<FocusRepository>() - .requestFindRefSearchFocus(selectAll: true); - } + _requestSearchFocus(context, state.currentScreen);Also applies to: 254-264, 288-298
lib/indexing/services/indexing_isolate_service.dart (1)
210-213: Consider making commit frequency configurableCommitting every 10 books might be too frequent for large libraries (causing performance overhead) or too infrequent for small libraries (losing progress on failure).
Consider making this configurable or adaptive based on library size:
+ // Adaptive commit frequency based on library size + final commitFrequency = totalBooks < 50 ? 5 : + totalBooks < 200 ? 10 : + totalBooks < 500 ? 20 : 50; + // ביצוע commit מדי פעם כדי לשחרר לוקים - if (processedBooks % 10 == 0) { + if (processedBooks % commitFrequency == 0) { await searchEngine?.commit(); await refEngine?.commit(); }lib/indexing/repository/indexing_repository.dart (1)
62-129: Unused method _indexTextBook can be removedThe
_indexTextBookmethod appears to be legacy code that's no longer used since indexing has been moved to the isolate service.Consider removing this method along with
_indexPdfBook(lines 132-173) since they're no longer called and their functionality has been moved to the isolate service. This will improve code maintainability by removing dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
lib/indexing/repository/indexing_repository.dart(5 hunks)lib/indexing/services/indexing_isolate_service.dart(1 hunks)lib/navigation/main_window_screen.dart(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_macos
- GitHub Check: build_linux
- GitHub Check: build_windows
- GitHub Check: build_android
🔇 Additional comments (8)
lib/navigation/main_window_screen.dart (6)
10-10: LGTM! Proper isolate management import added.The import for
IsolateManageris appropriately added to handle cleanup of background isolates on disposal.
22-22: LGTM! Navigation updated to use MoreScreen.The import and usage of
MoreScreenproperly replaces the previous favorites screen, aligning with the broader navigation refactoring in this PR.
148-150: UI label properly updated from favorites to "more".The navigation destination correctly uses the "more" icon (
Icons.more_horiz) with Hebrew label "עוד" (meaning "more"), consistent with the screen replacement.
270-270: Visual separator added for landscape mode.The VerticalDivider provides a clean visual separation between the navigation rail and content in landscape orientation.
357-357: Trailing newline properly maintained.Good practice to maintain the trailing newline at the end of the file.
43-43: Approve MoreScreen replacement
MoreScreen exists, initializes CalendarCubit with SettingsRepository, and imports/uses CalendarWidget as expected—dependencies are in place.lib/indexing/services/indexing_isolate_service.dart (1)
389-410: Potential infinite retry loop on persistent failuresThe retry logic in
search()andcount()methods could lead to an infinite loop if_initEngine()keeps failing.Add a retry limit to prevent infinite loops:
+ int _retryCount = 0; + static const int _maxRetries = 3; + Future<List<SearchResult>> search({ // ... parameters ... }) async { try { return await _engine.search( // ... parameters ... ); } catch (e) { + if (_retryCount >= _maxRetries) { + debugPrint('Max retries reached for search operation'); + rethrow; + } + _retryCount++; // אם יש בעיה בגישה, ננסה לפתוח מחדש _initEngine(); - return await _engine.search( + final result = await _engine.search( // ... parameters ... ); + _retryCount = 0; // Reset on success + return result; } }Apply similar changes to the
count()method at lines 419-436.Likely an incorrect or invalid review comment.
lib/indexing/repository/indexing_repository.dart (1)
80-82: Good addition of yielding controlThe periodic yielding with
Future.delayed(Duration.zero)is a good practice to prevent UI blocking during intensive operations.Also applies to: 152-154
| /// Cancels the ongoing indexing process. | ||
| void cancelIndexing() { | ||
| _tantivyDataProvider.isIndexing.value = false; | ||
| IndexingIsolateService.cancelIndexing(); |
There was a problem hiding this comment.
Missing await for async operation
IndexingIsolateService.cancelIndexing() is an async method but is called without await, which could lead to race conditions.
- void cancelIndexing() {
+ Future<void> cancelIndexing() async {
_tantivyDataProvider.isIndexing.value = false;
- IndexingIsolateService.cancelIndexing();
+ await IndexingIsolateService.cancelIndexing();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IndexingIsolateService.cancelIndexing(); | |
| Future<void> cancelIndexing() async { | |
| _tantivyDataProvider.isIndexing.value = false; | |
| await IndexingIsolateService.cancelIndexing(); | |
| } |
🤖 Prompt for AI Agents
In lib/indexing/repository/indexing_repository.dart around line 178,
IndexingIsolateService.cancelIndexing() is an async method being invoked without
awaiting; change the call to await IndexingIsolateService.cancelIndexing() and
propagate or handle the possible Future (e.g., mark the enclosing function as
async if not already, or capture and handle errors with try/catch) to ensure
cancellation completes before continuing and avoid race conditions.
| final indexPath = '${Settings.getValue('key-library-path') ?? 'C:/אוצריא'}${Platform.pathSeparator}index'; | ||
| final refIndexPath = '${Settings.getValue('key-library-path') ?? 'C:/אוצריא'}${Platform.pathSeparator}ref_index'; |
There was a problem hiding this comment.
Use Platform.pathSeparator correctly for path construction
The current implementation incorrectly uses Platform.pathSeparator to join path components. Platform.pathSeparator is the system's PATH separator (; on Windows, : on Unix), not the directory separator.
Apply this diff to fix the path construction:
- final indexPath = '${Settings.getValue('key-library-path') ?? 'C:/אוצריא'}${Platform.pathSeparator}index';
- final refIndexPath = '${Settings.getValue('key-library-path') ?? 'C:/אוצריא'}${Platform.pathSeparator}ref_index';
+ final libraryPath = Settings.getValue('key-library-path') ?? 'C:/אוצריא';
+ final indexPath = '$libraryPath${Platform.isWindows ? '\\' : '/'}index';
+ final refIndexPath = '$libraryPath${Platform.isWindows ? '\\' : '/'}ref_index';Or better yet, use the path package for proper cross-platform path handling:
import 'package:path/path.dart' as p;
final libraryPath = Settings.getValue('key-library-path') ?? 'C:/אוצריא';
final indexPath = p.join(libraryPath, 'index');
final refIndexPath = p.join(libraryPath, 'ref_index');🤖 Prompt for AI Agents
In lib/indexing/services/indexing_isolate_service.dart around lines 86-87 the
code incorrectly uses Platform.pathSeparator to concatenate directory components
(Platform.pathSeparator is for PATH entries, not directory separators); replace
the string concatenation with proper cross-platform path joins: retrieve the
libraryPath from Settings.getValue (with the same default), import package:path
as p, and build indexPath and refIndexPath using p.join(libraryPath, 'index')
and p.join(libraryPath, 'ref_index') so paths are constructed correctly on all
platforms.
| _isolateHandler!.responses.listen((message) { | ||
| if (message is IndexingProgressMessage) { | ||
| _progressController?.add(message); | ||
|
|
||
| if (message.isComplete || message.error != null) { | ||
| _isIndexing = false; | ||
| _progressController?.close(); | ||
| _progressController = null; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Potential memory leak: IsolateHandler responses subscription not disposed
The subscription to _isolateHandler!.responses is never cancelled, which could lead to a memory leak if the isolate is killed or replaced before completion.
Store and properly dispose of the subscription:
+ static StreamSubscription? _responseSubscription;
static Future<Stream<IndexingProgressMessage>> startIndexing(
Library library,
) async {
// ... existing code ...
// האזנה לתגובות מה-Isolate
- _isolateHandler!.responses.listen((message) {
+ _responseSubscription?.cancel();
+ _responseSubscription = _isolateHandler!.responses.listen((message) {
if (message is IndexingProgressMessage) {
_progressController?.add(message);
if (message.isComplete || message.error != null) {
_isIndexing = false;
_progressController?.close();
_progressController = null;
+ _responseSubscription?.cancel();
+ _responseSubscription = null;
}
}
});Also update the cancelIndexing method:
static Future<void> cancelIndexing() async {
+ _responseSubscription?.cancel();
+ _responseSubscription = null;
if (_isolateHandler != null) {
_isolateHandler!.send(CancelIndexingMessage());
await IsolateManager.kill('indexing');
_isolateHandler = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _isolateHandler!.responses.listen((message) { | |
| if (message is IndexingProgressMessage) { | |
| _progressController?.add(message); | |
| if (message.isComplete || message.error != null) { | |
| _isIndexing = false; | |
| _progressController?.close(); | |
| _progressController = null; | |
| } | |
| } | |
| }); | |
| // lib/indexing/services/indexing_isolate_service.dart | |
| import 'dart:async'; | |
| // ... other imports ... | |
| class IndexingIsolateService { | |
| static StreamSubscription? _responseSubscription; | |
| static Future<Stream<IndexingProgressMessage>> startIndexing( | |
| Library library, | |
| ) async { | |
| // ... existing setup code ... | |
| // האזנה לתגובות מה-Isolate | |
| _responseSubscription?.cancel(); | |
| _responseSubscription = _isolateHandler!.responses.listen((message) { | |
| if (message is IndexingProgressMessage) { | |
| _progressController?.add(message); | |
| if (message.isComplete || message.error != null) { | |
| _isIndexing = false; | |
| _progressController?.close(); | |
| _progressController = null; | |
| _responseSubscription?.cancel(); | |
| _responseSubscription = null; | |
| } | |
| } | |
| }); | |
| return _progressController!.stream; | |
| } | |
| static Future<void> cancelIndexing() async { | |
| _responseSubscription?.cancel(); | |
| _responseSubscription = null; | |
| if (_isolateHandler != null) { | |
| _isolateHandler!.send(CancelIndexingMessage()); | |
| await IsolateManager.kill('indexing'); | |
| _isolateHandler = null; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/indexing/services/indexing_isolate_service.dart around lines 96 to 106,
the subscription to _isolateHandler!.responses is never stored or cancelled
which can leak if the isolate is replaced; capture the returned
StreamSubscription in a private field (e.g. _responsesSubscription), replace the
current .listen(...) call with assignment to that field, and ensure you call
_responsesSubscription?.cancel() and set it to null when indexing completes,
when cancelling indexing (update cancelIndexing to cancel the subscription and
clear controllers/flags), and in any dispose/cleanup path so the subscription is
always properly disposed.
| id: BigInt.from(DateTime.now().microsecondsSinceEpoch), | ||
| title: title, | ||
| reference: refText, | ||
| shortRef: shortref, | ||
| segment: BigInt.from(i), | ||
| isPdf: false, | ||
| filePath: ''); |
There was a problem hiding this comment.
Non-unique ID generation risk
Using DateTime.now().microsecondsSinceEpoch for generating document IDs may produce duplicates when indexing is very fast, potentially causing document overwrites.
Consider using a counter or combining timestamp with additional entropy:
+ static int _docIdCounter = 0;
+
refEngine.addDocument(
- id: BigInt.from(DateTime.now().microsecondsSinceEpoch),
+ id: BigInt.from(DateTime.now().microsecondsSinceEpoch * 1000 + (_docIdCounter++ % 1000)),
title: title,
reference: refText,
shortRef: shortref,
segment: BigInt.from(i),
isPdf: false,
filePath: '');Apply the same fix to lines 301, 346 in this file.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/indexing/services/indexing_isolate_service.dart around lines 288-294 (and
apply same change at lines ~301 and ~346), the current ID creation uses
BigInt.from(DateTime.now().microsecondsSinceEpoch) which can collide when
indexing is very fast; replace this with a true unique generator (e.g., combine
the timestamp with an incrementing atomic counter or append random/UUID entropy)
and convert that result to BigInt for the document id so each generated id is
monotonic+unique even under high-throughput conditions.
| try { | ||
| // ניסיון לפתוח במצב קריאה | ||
| _engine = SearchEngine(path: indexPath); | ||
| } catch (e) { | ||
| debugPrint('Failed to open search engine in read-only mode: $e'); | ||
| rethrow; | ||
| } | ||
| } |
There was a problem hiding this comment.
ReadOnlySearchEngine doesn't enforce read-only access
The class name suggests read-only access, but it creates a SearchEngine instance which might have write capabilities. This could lead to unintended modifications.
The SearchEngine constructor should accept a read-only flag or you should verify that the underlying implementation prevents writes. If the SearchEngine doesn't support read-only mode, consider documenting this limitation clearly:
void _initEngine() {
try {
- // ניסיון לפתוח במצב קריאה
+ // Note: SearchEngine doesn't enforce read-only mode
+ // This wrapper ensures only read operations are exposed
_engine = SearchEngine(path: indexPath);
} catch (e) {
debugPrint('Failed to open search engine in read-only mode: $e');
rethrow;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // ניסיון לפתוח במצב קריאה | |
| _engine = SearchEngine(path: indexPath); | |
| } catch (e) { | |
| debugPrint('Failed to open search engine in read-only mode: $e'); | |
| rethrow; | |
| } | |
| } | |
| void _initEngine() { | |
| try { | |
| // Note: SearchEngine doesn't enforce read-only mode | |
| // This wrapper ensures only read operations are exposed | |
| _engine = SearchEngine(path: indexPath); | |
| } catch (e) { | |
| debugPrint('Failed to open search engine in read-only mode: $e'); | |
| rethrow; | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/indexing/services/indexing_isolate_service.dart around lines 371 to 378,
the ReadOnlySearchEngine currently constructs a SearchEngine without enforcing
read-only semantics; update the code to either (1) use a SearchEngine
constructor or factory that accepts an explicit readOnly/readOnlyMode flag and
pass true, or (2) wrap/replace the instance with a read-only facade that only
exposes read methods and blocks mutating calls, and add a runtime assertion that
prevents any write API from being called; if the underlying SearchEngine cannot
be used read-only, update the class and public docs/comments to clearly state
the limitation instead of naming it ReadOnlySearchEngine.
| // סגירת כל ה-Isolates בעת יציאה מהאפליקציה | ||
| _cleanupIsolates(); | ||
| super.dispose(); | ||
| } | ||
|
|
||
| /// ניקוי כל ה-Isolates | ||
| void _cleanupIsolates() async { | ||
| try { | ||
| await IsolateManager.killAll(); | ||
| } catch (e) { | ||
| // אם יש שגיאה בניקוי, נמשיך בכל זאת | ||
| debugPrint('Error cleaning up isolates: $e'); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure all isolates are properly terminated before disposal.
The cleanup implementation looks good with proper error handling. The Hebrew comments clearly document the purpose.
🏁 Script executed:
#!/bin/bash
# Verify IsolateManager implementation and killAll method
ast-grep --pattern 'class IsolateManager {
$$$
static $_ killAll($$$) {
$$$
}
$$$
}'
# Check for other isolate usages that need cleanup
rg -n "Isolate\." --type dart -C 3Length of output: 12980
Implement missing IsolateManager.killAll
main_window_screen.dart calls IsolateManager.killAll(), but no static killAll method exists in lib/utils/isolate_manager.dart, causing a compile error. Add a static Future<void> killAll() that iterates over tracked isolates and disposes each.
🤖 Prompt for AI Agents
In lib/navigation/main_window_screen.dart around lines 63-76 it calls
IsolateManager.killAll(), but lib/utils/isolate_manager.dart lacks that static
method; add a static Future<void> killAll() to lib/utils/isolate_manager.dart
that asynchronously iterates the manager's tracked isolates list (e.g.,
_trackedIsolates), attempts to dispose/kill each isolate with per-isolate
try/catch to avoid stopping on errors, awaits each disposal, clears the tracked
list afterward, and propagates or logs errors as appropriate so the caller can
await cleanup without compile errors.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/empty_library/bloc/empty_library_bloc.dart (1)
228-233: Harden against Zip Slip path traversalUsing
'$libraryPath/$filename'directly lets malicious entries escape the target directory (e.g.,../../somewhere). Normalize and validate paths before writing.Add the import:
import 'package:path/path.dart' as p;Then replace the path building logic:
final libRoot = p.normalize(p.absolute(libraryPath)); final name = p.normalize(file.name); if (p.isAbsolute(name)) { throw Exception('נתיב לא חוקי בארכיון: $name'); } final outPath = p.normalize(p.absolute(p.join(libraryPath, name))); if (!p.isWithin(libRoot, outPath)) { throw Exception('Zip entry attempts path traversal: $name'); } // use outPath instead of '$libraryPath/$filename'
♻️ Duplicate comments (6)
lib/indexing/repository/indexing_repository.dart (2)
41-45: Don't mark both types and don't mark "done" at book startYou add both textBook/pdfBook and do it when a book starts processing, not when it completes. This mis-tracks progress and can skip partially indexed books next runs. Include bookType + bookCompleted in the progress message and only persist when completed.
Apply in this file:
- if (progress.currentBook != null) { - _tantivyDataProvider.booksDone.add('${progress.currentBook}textBook'); - _tantivyDataProvider.booksDone.add('${progress.currentBook}pdfBook'); - saveIndexedBooks(); - } + if (progress.currentBook != null && + progress.bookCompleted == true && + progress.bookType != null) { + final key = progress.bookType == BookType.text + ? '${progress.currentBook}textBook' + : '${progress.currentBook}pdfBook'; + if (!_tantivyDataProvider.booksDone.contains(key)) { + _tantivyDataProvider.booksDone.add(key); + saveIndexedBooks(); + } + }Note: See my comments in indexing_isolate_service.dart to add bookType + bookCompleted fields and to emit a completion event per book.
173-177: Await async cancellationcancelIndexing() calls an async method without await; make the API async and await to avoid races.
- void cancelIndexing() { + Future<void> cancelIndexing() async { _tantivyDataProvider.isIndexing.value = false; - IndexingIsolateService.cancelIndexing(); + await IndexingIsolateService.cancelIndexing(); } ``` <!-- review_comment_end --> </blockquote></details> <details> <summary>lib/indexing/services/indexing_isolate_service.dart (4)</summary><blockquote> `371-379`: **ReadOnlySearchEngine isn't actually read-only** This wrapper still constructs a writable SearchEngine. If the engine supports readOnly mode, pass it; otherwise document the limitation and keep the facade read-only. ```diff void _initEngine() { try { - // ניסיון לפתוח במצב קריאה - _engine = SearchEngine(path: indexPath); + // Note: SearchEngine lacks an explicit read-only flag; this facade exposes only read ops. + _engine = SearchEngine(path: indexPath); } catch (e) { debugPrint('Failed to open search engine in read-only mode: $e'); rethrow; } } ``` <!-- review_comment_end --> --- `86-88`: **Use proper path joining (not Platform.pathSeparator)** pathSeparator is for PATH entries, not directories. Use package:path. ```diff +import 'package:path/path.dart' as p; @@ - final indexPath = '${Settings.getValue('key-library-path') ?? 'C:/אוצריא'}${Platform.pathSeparator}index'; - final refIndexPath = '${Settings.getValue('key-library-path') ?? 'C:/אוצריא'}${Platform.pathSeparator}ref_index'; + final libraryPath = (Settings.getValue('key-library-path') ?? 'C:/אוצריא') as String; + final indexPath = p.join(libraryPath, 'index'); + final refIndexPath = p.join(libraryPath, 'ref_index'); ``` <!-- review_comment_end --> --- `96-106`: **Leak: response subscription not stored/cancelled** Keep the StreamSubscription and dispose it on completion/cancel to prevent leaks. ```diff + static StreamSubscription? _responsesSub; @@ - _isolateHandler!.responses.listen((message) { + _responsesSub?.cancel(); + _responsesSub = _isolateHandler!.responses.listen((message) { if (message is IndexingProgressMessage) { _progressController?.add(message); if (message.isComplete || message.error != null) { _isIndexing = false; _progressController?.close(); _progressController = null; + _responsesSub?.cancel(); + _responsesSub = null; } } }); ``` <!-- review_comment_end --> --- `287-295`: **Risk of ID collisions under high throughput** microsecondsSinceEpoch can collide; add a tiny counter for uniqueness. Add helper (place near top-level in this file): ```dart // Unique, monotonic-ish BigInt id int _docIdCounter = 0; BigInt _nextId() => BigInt.from((DateTime.now().microsecondsSinceEpoch << 12) | (_docIdCounter++ & 0xFFF));Use it here:
- refEngine.addDocument( - id: BigInt.from(DateTime.now().microsecondsSinceEpoch), + refEngine.addDocument( + id: _nextId(), @@ - searchEngine.addDocument( - id: BigInt.from(DateTime.now().microsecondsSinceEpoch), + searchEngine.addDocument( + id: _nextId(), @@ - searchEngine.addDocument( - id: BigInt.from(DateTime.now().microsecondsSinceEpoch), + searchEngine.addDocument( + id: _nextId(), ``` <!-- review_comment_end --> Also applies to: 300-309, 346-355 </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>lib/empty_library/bloc/empty_library_bloc.dart (2)</summary><blockquote> `232-236`: **Progress never reaches 100% (off-by-one)** You emit progress before incrementing `extractedFiles`, so the last update is `< 1.0`. Emit after a successful write (or compute `(extractedFiles + 1) / totalFiles`): ```dart try { if (file.isFile) { // ... write ... } else { await Directory(filePath).create(recursive: true); } extractedFiles++; add(DownloadProgressUpdated( progress: extractedFiles / totalFiles, currentOperation: 'מחלץ: $filename', downloadedMB: state.downloadedMB, downloadSpeed: state.downloadSpeed, )); } catch (e) { ... }
225-225: Comment tweak (optional)If you adopt streaming again, consider updating the comment to “Stream and extract entries one at a time.”
lib/indexing/repository/indexing_repository.dart (1)
31-35: On error, cancel the isolateAvoid leaving the indexing isolate alive on error.
if (progress.error != null) { debugPrint('Indexing error: ${progress.error}'); _tantivyDataProvider.isIndexing.value = false; + await IndexingIsolateService.cancelIndexing(); throw Exception(progress.error); } ``` <!-- review_comment_end --> </blockquote></details> <details> <summary>lib/indexing/services/indexing_isolate_service.dart (1)</summary><blockquote> `137-146`: **Avoid preloading entire text in UI isolate** Loading large text via `await book.text` before spawning the isolate increases memory and blocks the UI isolate for I/O. Prefer passing metadata/path and load inside the indexing isolate. I can propose a minimal change set to move text loading into the isolate if you can share how TextBook exposes its storage (path/URI/stream). <!-- review_comment_end --> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 826df8093df47b0c09c80018441b174df59203c6 and 192a7fb7a1f8e9347fe9a81c0e2c0b40392a03c4. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pubspec.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (5)</summary> * `lib/data/data_providers/file_system_data_provider.dart` (4 hunks) * `lib/empty_library/bloc/empty_library_bloc.dart` (2 hunks) * `lib/indexing/repository/indexing_repository.dart` (5 hunks) * `lib/indexing/services/indexing_isolate_service.dart` (1 hunks) * `lib/pdf_book/pdf_book_screen.dart` (18 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * lib/data/data_providers/file_system_data_provider.dart * lib/pdf_book/pdf_book_screen.dart </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)</summary> * GitHub Check: build_linux * GitHub Check: build_windows * GitHub Check: build_macos * GitHub Check: build_android </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>lib/indexing/repository/indexing_repository.dart (2)</summary><blockquote> `76-79`: **Nice: cooperative yielding for text indexing** Yielding every 100 lines reduces UI jank during long loops. <!-- review_comment_end --> --- `150-152`: **Nice: cooperative yielding for PDF indexing** Periodic yield in the inner loop keeps the event loop responsive. <!-- review_comment_end --> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| // Read the zip file and decode it | ||
| final bytes = await _tempFile!.readAsBytes(); | ||
| final archive = ZipDecoder().decodeBytes(bytes); | ||
| final totalFiles = archive.files.length; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Streaming extraction required to avoid OOM/UI jank
readAsBytes + decodeBytes loads the entire ZIP and each entry fully into memory; writeAsBytes(file.content) also materializes file contents. Large archives will spike RAM and can freeze the main isolate. Use the archive_io streaming APIs instead.
Apply this diff within the shown ranges:
- // Read the zip file and decode it
- final bytes = await _tempFile!.readAsBytes();
- final archive = ZipDecoder().decodeBytes(bytes);
+ // Stream the zip and decode without loading it all to RAM
+ final inputStream = InputFileStream(_tempFile!.path);
+ final archive = ZipDecoder().decodeBuffer(inputStream);
@@
- await outputFile.writeAsBytes(file.content);
+ final output = OutputFileStream(filePath);
+ try {
+ file.writeContent(output); // streamed decompression
+ } finally {
+ output.close();
+ }Also ensure the InputFileStream is closed after the loop:
// after the for (...) loop
inputStream.close();If you keep non-streaming decode for any reason, at least offload it to an isolate (Isolate.run/compute) to keep the UI responsive.
Also applies to: 241-241
| class IndexingProgressMessage { | ||
| final int processed; | ||
| final int total; | ||
| final String? currentBook; | ||
| final bool isComplete; | ||
| final String? error; | ||
|
|
||
| IndexingProgressMessage({ | ||
| required this.processed, | ||
| required this.total, | ||
| this.currentBook, | ||
| this.isComplete = false, | ||
| this.error, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Progress model lacks granularity (need bookType + bookCompleted)
Repository needs to know which type finished and only then persist. Extend the message.
class IndexingProgressMessage {
final int processed;
final int total;
final String? currentBook;
+ final BookType? bookType;
+ final bool bookCompleted;
final bool isComplete;
final String? error;
IndexingProgressMessage({
required this.processed,
required this.total,
this.currentBook,
+ this.bookType,
+ this.bookCompleted = false,
this.isComplete = false,
this.error,
});
}
``` <!-- review_comment_end -->
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
class IndexingProgressMessage {
final int processed;
final int total;
final String? currentBook;
final BookType? bookType;
final bool bookCompleted;
final bool isComplete;
final String? error;
IndexingProgressMessage({
required this.processed,
required this.total,
this.currentBook,
this.bookType,
this.bookCompleted = false,
this.isComplete = false,
this.error,
});
}🤖 Prompt for AI Agents
In lib/indexing/services/indexing_isolate_service.dart around lines 30 to 44,
the IndexingProgressMessage model needs two new fields so the repository can
know which book type finished before persisting: add a nullable String bookType
(or an enum if preferred) and a bool bookCompleted with a default of false,
include them in the constructor and field declarations, and update any places
that construct or read IndexingProgressMessage (including
serialization/deserialization or tests) to set and respect these new fields.
| /// ביטול תהליך האינדוקס | ||
| static Future<void> cancelIndexing() async { | ||
| if (_isolateHandler != null) { | ||
| _isolateHandler!.send(CancelIndexingMessage()); | ||
| await IsolateManager.kill('indexing'); | ||
| _isolateHandler = null; | ||
| } | ||
|
|
||
| _isIndexing = false; | ||
| _progressController?.close(); | ||
| _progressController = null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden cancelIndexing() cleanup
Also cancel the responses sub and guard double-close on the controller.
static Future<void> cancelIndexing() async {
+ _responsesSub?.cancel();
+ _responsesSub = null;
if (_isolateHandler != null) {
_isolateHandler!.send(CancelIndexingMessage());
await IsolateManager.kill('indexing');
_isolateHandler = null;
}
_isIndexing = false;
- _progressController?.close();
+ if (!(_progressController?.isClosed ?? true)) {
+ await _progressController!.close();
+ }
_progressController = null;
}
``` <!-- review_comment_end -->
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
/// ביטול תהליך האינדוקס
static Future<void> cancelIndexing() async {
// Cancel any outstanding response subscription before killing the isolate
_responsesSub?.cancel();
_responsesSub = null;
if (_isolateHandler != null) {
_isolateHandler!.send(CancelIndexingMessage());
await IsolateManager.kill('indexing');
_isolateHandler = null;
}
_isIndexing = false;
// Only close the controller if it isn’t already closed, and await its closure
if (!(_progressController?.isClosed ?? true)) {
await _progressController!.close();
}
_progressController = null;
}🤖 Prompt for AI Agents
In lib/indexing/services/indexing_isolate_service.dart around lines 118 to 129,
cancelIndexing() currently sends cancel and kills the isolate then closes the
progress controller without cancelling the responses subscription or guarding
against double-close; update it to cancel and null out the responses
subscription (e.g. _responsesSub?.cancel(); _responsesSub = null;) before/after
killing the isolate, and close the controller only if it exists and is not
already closed (e.g. if (_progressController != null &&
!_progressController!.isClosed) { await _progressController!.close(); }), then
set _progressController = null and _isIndexing = false as before.
| for (final book in message.books) { | ||
| if (shouldCancel) break; | ||
|
|
||
| try { | ||
| // שליחת עדכון התקדמות | ||
| context.send(IndexingProgressMessage( | ||
| processed: processedBooks, | ||
| total: totalBooks, | ||
| currentBook: book.title, | ||
| )); | ||
|
|
||
| // אינדוקס הספר |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Emit start and completion events per book
Send a "start" event (bookCompleted=false) and a "done" event (bookCompleted=true) after each book to let the repository persist correctly.
- // שליחת עדכון התקדמות
+ // שליחת עדכון התחלה לספר
context.send(IndexingProgressMessage(
processed: processedBooks,
total: totalBooks,
currentBook: book.title,
+ bookType: book.type,
+ bookCompleted: false,
));
@@
- processedBooks++;
+ processedBooks++;
+ // שליחת עדכון סיום לספר
+ context.send(IndexingProgressMessage(
+ processed: processedBooks,
+ total: totalBooks,
+ currentBook: book.title,
+ bookType: book.type,
+ bookCompleted: true,
+ ));
// ביצוע commit מדי פעם כדי לשחרר לוקים
if (processedBooks % 10 == 0) {
await searchEngine?.commit();
await refEngine?.commit();
}
``` <!-- review_comment_end -->
Also applies to: 207-214
<details>
<summary>🤖 Prompt for AI Agents</summary>
In lib/indexing/services/indexing_isolate_service.dart around lines 180-191 (and
similarly at 207-214), the loop currently only sends a progress update; emit two
repository events per book: send a "start" event before processing the book with
bookCompleted=false, then after the book is fully indexed send a "done" event
with bookCompleted=true so the repository can persist state correctly; implement
by creating and sending IndexingProgressMessage (or the repository event type
used) with bookCompleted=false immediately before indexing logic and another
with bookCompleted=true right after successful indexing (ensure you still send
the existing processed/total/currentBook fields and handle
cancellation/exception paths appropriately).
</details>
<!-- fingerprinting:phantom:poseidon:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
| Future<void> _indexTextBookInIsolate( | ||
| BookData book, | ||
| SearchEngine searchEngine, | ||
| ReferenceSearchEngine refEngine, | ||
| Set<String> booksDone, | ||
| ) async { | ||
| // בדיקה אם כבר אונדקס | ||
| final bookKey = "${book.title}textBook"; | ||
| if (booksDone.contains(bookKey)) return; | ||
|
|
||
| final text = book.textContent ?? ''; | ||
| final title = book.title; | ||
| final topics = "/${book.topics.replaceAll(', ', '/')}"; | ||
|
|
||
| final texts = text.split('\n'); | ||
| List<String> reference = []; | ||
|
|
||
| for (int i = 0; i < texts.length; i++) { | ||
| // תן לאירועים אחרים לרוץ | ||
| if (i % 100 == 0) { | ||
| await Future.delayed(Duration.zero); | ||
| } | ||
|
|
||
| String line = texts[i]; | ||
|
|
||
| if (line.startsWith('<h')) { | ||
| // עדכון הרפרנס | ||
| if (reference.isNotEmpty && | ||
| reference.any((element) => | ||
| element.substring(0, 4) == line.substring(0, 4))) { | ||
| reference.removeRange( | ||
| reference.indexWhere((element) => | ||
| element.substring(0, 4) == line.substring(0, 4)), | ||
| reference.length); | ||
| } | ||
| reference.add(line); | ||
|
|
||
| // אינדוקס כרפרנס | ||
| String refText = stripHtmlIfNeeded(reference.join(" ")); | ||
| final shortref = replaceParaphrases(removeSectionNames(refText)); | ||
|
|
||
| refEngine.addDocument( | ||
| id: BigInt.from(DateTime.now().microsecondsSinceEpoch), | ||
| title: title, | ||
| reference: refText, | ||
| shortRef: shortref, | ||
| segment: BigInt.from(i), | ||
| isPdf: false, | ||
| filePath: ''); | ||
| } else { | ||
| line = stripHtmlIfNeeded(line); | ||
| line = removeVolwels(line); | ||
|
|
||
| // הוספה לאינדקס | ||
| searchEngine.addDocument( | ||
| id: BigInt.from(DateTime.now().microsecondsSinceEpoch), | ||
| title: title, | ||
| reference: stripHtmlIfNeeded(reference.join(', ')), | ||
| topics: '$topics/$title', | ||
| text: line, | ||
| segment: BigInt.from(i), | ||
| isPdf: false, | ||
| filePath: ''); | ||
| } | ||
| } | ||
|
|
||
| booksDone.add(bookKey); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cancellation is coarse-grained; add checks inside loops
Currently cancel only breaks between books. Pass a cancellation callback and check inside the inner loops to stop promptly.
Future<void> _indexTextBookInIsolate(
BookData book,
SearchEngine searchEngine,
ReferenceSearchEngine refEngine,
- Set<String> booksDone,
+ Set<String> booksDone,
+ bool Function() isCancelled,
) async {
@@
- for (int i = 0; i < texts.length; i++) {
+ for (int i = 0; i < texts.length; i++) {
+ if (isCancelled()) return;
// תן לאירועים אחרים לרוץ
if (i % 100 == 0) {
await Future.delayed(Duration.zero);
+ if (isCancelled()) return;
}And pass the callback from the caller:
- await _indexTextBookInIsolate(
+ await _indexTextBookInIsolate(
book,
searchEngine!,
refEngine!,
- booksDone,
+ booksDone,
+ () => shouldCancel,
);
``` <!-- review_comment_end -->
<details>
<summary>🤖 Prompt for AI Agents</summary>
In lib/indexing/services/indexing_isolate_service.dart around lines 246 to 314,
the function _indexTextBookInIsolate needs cooperative cancellation: add a
cancellation callback parameter (e.g. bool Function() isCancelled or a
CancellationToken) to the function signature and to the caller, check it
immediately at the start and inside the texts for-loop (and where you await
Future.delayed or before each expensive addDocument call) and return early if it
indicates cancellation; ensure you do not add the bookKey to booksDone when
cancelled (only add after successful completion) and make sure any partial state
is consistent before returning.
</details>
<!-- fingerprinting:phantom:poseidon:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
| Future<void> _indexPdfBookInIsolate( | ||
| BookData book, | ||
| SearchEngine searchEngine, | ||
| Set<String> booksDone, | ||
| ) async { | ||
| // בדיקה אם כבר אונדקס | ||
| final bookKey = "${book.title}pdfBook"; | ||
| if (booksDone.contains(bookKey)) return; | ||
|
|
||
| final document = await PdfDocument.openFile(book.path!); | ||
| final pages = document.pages; | ||
| final outline = await document.loadOutline(); | ||
| final title = book.title; | ||
| final topics = "/${book.topics.replaceAll(', ', '/')}"; | ||
|
|
||
| for (int i = 0; i < pages.length; i++) { | ||
| final pageText = await pages[i].loadText(); | ||
| final texts = pageText.split('\n'); | ||
|
|
||
| for (int j = 0; j < texts.length; j++) { | ||
| // תן לאירועים אחרים לרוץ | ||
| if (j % 50 == 0) { | ||
| await Future.delayed(Duration.zero); | ||
| } | ||
|
|
||
| final bookmark = await refFromPageNumber(i + 1, outline, title); | ||
| final ref = bookmark.isNotEmpty | ||
| ? '$title, $bookmark, עמוד ${i + 1}' | ||
| : '$title, עמוד ${i + 1}'; | ||
|
|
||
| searchEngine.addDocument( | ||
| id: BigInt.from(DateTime.now().microsecondsSinceEpoch), | ||
| title: title, | ||
| reference: ref, | ||
| topics: '$topics/$title', | ||
| text: texts[j], | ||
| segment: BigInt.from(i), | ||
| isPdf: true, | ||
| filePath: book.path!); | ||
| } | ||
| } | ||
|
|
||
| booksDone.add(bookKey); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
PDF: compute bookmark once per page and add cancel checks
Avoid recomputing the same bookmark for every line; also add cancellation checks like in text flow.
Future<void> _indexPdfBookInIsolate(
BookData book,
SearchEngine searchEngine,
- Set<String> booksDone,
+ Set<String> booksDone,
+ bool Function() isCancelled,
) async {
@@
- for (int i = 0; i < pages.length; i++) {
+ for (int i = 0; i < pages.length; i++) {
+ if (isCancelled()) return;
final pageText = await pages[i].loadText();
final texts = pageText.split('\n');
-
- for (int j = 0; j < texts.length; j++) {
+ final bookmark = await refFromPageNumber(i + 1, outline, title);
+ for (int j = 0; j < texts.length; j++) {
// תן לאירועים אחרים לרוץ
if (j % 50 == 0) {
await Future.delayed(Duration.zero);
+ if (isCancelled()) return;
}
-
- final bookmark = await refFromPageNumber(i + 1, outline, title);
final ref = bookmark.isNotEmpty
? '$title, $bookmark, עמוד ${i + 1}'
: '$title, עמוד ${i + 1}';And pass the callback from the caller:
- await _indexPdfBookInIsolate(
+ await _indexPdfBookInIsolate(
book,
searchEngine!,
- booksDone,
+ booksDone,
+ () => shouldCancel,
);
``` <!-- review_comment_end -->
Also applies to: 331-355
<details>
<summary>🤖 Prompt for AI Agents</summary>
In lib/indexing/services/indexing_isolate_service.dart around lines 316 to 360,
the current loop recomputes the bookmark for every line and lacks cancellation
checks and a passed cancel callback; compute the bookmark once per page before
iterating the page's lines, add periodic cancellation checks (similar to the
text flow implementation) inside the inner loop and before heavy work so the
isolate can stop promptly, and modify the function signature to accept the
caller's cancellation callback (propagate and call it where appropriate) so
indexing can be aborted cleanly.
</details>
<!-- fingerprinting:phantom:poseidon:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/main.dart (1)
42-44: sqflite FFI import — verify non-desktop builds; consider conditionalization.Ensure web/mobile builds still compile. If not, move FFI-specific imports behind conditional imports.
Example structure (new files):
// lib/notes/db_factory.dart export 'db_factory_stub.dart' if (dart.library.io) 'db_factory_desktop.dart'; // lib/notes/db_factory_desktop.dart import 'package:sqflite_common_ffi/sqflite_ffi.dart'; void initDbFactory() { sqfliteFfiInit(); databaseFactory = databaseFactoryFfi; } // lib/notes/db_factory_stub.dart void initDbFactory() {}Then in initialize():
import 'package:otzaria/notes/db_factory.dart'; initDbFactory();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/main.dart(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_android
- GitHub Check: build_macos
- GitHub Check: build_linux
- GitHub Check: build_windows
🔇 Additional comments (2)
lib/main.dart (2)
159-164: SQLite FFI init on desktop — LGTM.Correct platform gating and factory wiring.
131-133: Unconditional registration of NotesBloc is safe —initialize()already catches and suppresses database‐initialization failures, and allNotesBlocdatabase operations are wrapped in try/catch within its event handlers, emittingNotesErrorrather than crashing. Ignore the suggested conditional provider.Likely an incorrect or invalid review comment.
| import 'package:flutter/foundation.dart'; | ||
| import 'package:flutter/material.dart'; | ||
| import 'package:flutter/services.dart'; | ||
| import 'package:flutter_single_instance/flutter_single_instance.dart'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard single-instance logic to desktop; avoid exit on mobile/web.
The plugin is typically desktop-focused; calling exit(0) on mobile/web can be problematic. Gate the check by platform.
- // Check for single instance
- FlutterSingleInstance flutterSingleInstance = FlutterSingleInstance();
- bool isFirstInstance = await flutterSingleInstance.isFirstInstance();
- if (!isFirstInstance) {
- // If not the first instance, exit the app
- exit(0);
- }
+ // Check for single instance (desktop only)
+ if (!kIsWeb && (Platform.isWindows || Platform.isLinux || Platform.isMacOS)) {
+ final flutterSingleInstance = FlutterSingleInstance();
+ final isFirstInstance = await flutterSingleInstance.isFirstInstance();
+ if (!isFirstInstance) {
+ // Secondary instance exits; primary remains.
+ exit(0);
+ }
+ }Also applies to: 79-86
| // Initialize notes database | ||
| try { | ||
| await DatabaseSchema.initializeDatabase(); | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('Failed to initialize notes database: $e'); | ||
| } | ||
| // Continue without notes functionality if database fails | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t swallow notes DB init failures; log in release and gate features.
Right now failures are silent in release and NotesBloc is still registered. Add release logging here and gate notes features (see next comment).
try {
await DatabaseSchema.initializeDatabase();
} catch (e) {
if (kDebugMode) {
print('Failed to initialize notes database: $e');
+ } else {
+ // Persist the failure in release builds too
+ File('errors.txt').writeAsStringSync(
+ 'Failed to initialize notes database: $e\n',
+ mode: FileMode.append,
+ );
}
// Continue without notes functionality if database fails
}🤖 Prompt for AI Agents
In lib/main.dart around lines 172 to 180, the current try/catch only logs DB
init failures in debug and swallows errors in release while still registering
NotesBloc; change it to always log the error (use process-level logging or
Flutter's logging so release builds record the failure) and propagate/record a
boolean flag (e.g., notesAvailable = false) when initialization fails; then
ensure NotesBloc registration and any notes-related UI/features are gated behind
that flag so the app doesn't register or enable notes functionality when the
database failed to initialize.
| if (libraryPath == null && (Platform.isLinux || Platform.isMacOS)) { | ||
| // Use the working directory for Linux and macOS | ||
| await Settings.setValue( | ||
| 'key-library-path', '.'); | ||
| await Settings.setValue('key-library-path', '.'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Linux/macOS default path: avoid '.'; use a stable per-user app directory.
Using CWD is brittle (varies by launcher; may be read-only). Prefer getApplicationSupportDirectory().
- if (libraryPath == null && (Platform.isLinux || Platform.isMacOS)) {
- // Use the working directory for Linux and macOS
- await Settings.setValue('key-library-path', '.');
- }
+ if (libraryPath == null && (Platform.isLinux || Platform.isMacOS)) {
+ // Use the per-user application support directory on Linux/macOS
+ final supportDir = await getApplicationSupportDirectory();
+ await Settings.setValue('key-library-path', supportDir.path);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (libraryPath == null && (Platform.isLinux || Platform.isMacOS)) { | |
| // Use the working directory for Linux and macOS | |
| await Settings.setValue( | |
| 'key-library-path', '.'); | |
| await Settings.setValue('key-library-path', '.'); | |
| } | |
| if (libraryPath == null && (Platform.isLinux || Platform.isMacOS)) { | |
| // Use the per-user application support directory on Linux/macOS | |
| final supportDir = await getApplicationSupportDirectory(); | |
| await Settings.setValue('key-library-path', supportDir.path); | |
| } |
🤖 Prompt for AI Agents
In lib/main.dart around lines 222-225, avoid setting the library path to the
current working directory '.' for Linux/macOS; instead obtain and use a stable
per-user application support directory (via path_provider's
getApplicationSupportDirectory()), ensure the directory exists (create it if
missing), and pass directory.path to Settings.setValue('key-library-path'); also
add the necessary import for path_provider and handle the async/failure case
with a sensible fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/indexing/repository/indexing_repository.dart (4)
45-47: Bug: Book marked “done” even if indexing was cancelled mid-runIf
_indexTextBook/_indexPdfBookreturn early due to cancellation, the code still adds the${title}{type}key tobooksDone, causing future runs to skip an incompletely indexed book.Minimal fix (guard on the flag):
- await _indexTextBook(book); - _tantivyDataProvider.booksDone.add("${book.title}textBook"); + await _indexTextBook(book); + if (_tantivyDataProvider.isIndexing.value) { + _tantivyDataProvider.booksDone.add("${book.title}textBook"); + }Preferable fix (propagate completion state):
- Future<void> _indexTextBook(TextBook book) async { + Future<bool> _indexTextBook(TextBook book) async { ... - if (!_tantivyDataProvider.isIndexing.value) { - return; + if (!_tantivyDataProvider.isIndexing.value) { + return false; } ... await index.commit(); await refIndex.commit(); saveIndexedBooks(); - } + return true; + }And call sites:
- await _indexTextBook(book); - _tantivyDataProvider.booksDone.add("${book.title}textBook"); + final done = await _indexTextBook(book); + if (done) { + _tantivyDataProvider.booksDone.add("${book.title}textBook"); + }Apply the same pattern to
_indexPdfBook.Also applies to: 56-58
53-53: Avoid reading entire PDF into memory to compute SHA-1
readAsBytes()can OOM on large PDFs. Stream the file into the hash instead.- if (_tantivyDataProvider.booksDone.contains( - sha1.convert(await File(book.path).readAsBytes()).toString())) { + final digest = await sha1.bind(File(book.path).openRead()).first; + if (_tantivyDataProvider.booksDone.contains(digest.toString())) {Optionally, also record this digest upon successful indexing to make the check effective on subsequent runs.
169-191: Compute page bookmark once per page, not per line
refFromPageNumberis the same for every line on a page; calling it inside the inner loop is O(lines_per_page) redundant work.- for (int i = 0; i < pages.length; i++) { - final texts = (await pages[i].loadText()).fullText.split('\n'); - // Index each line from the page - for (int j = 0; j < texts.length; j++) { + for (int i = 0; i < pages.length; i++) { + final texts = (await pages[i].loadText()).fullText.split('\n'); + final bookmark = await refFromPageNumber(i + 1, outline, title); + final pageRef = bookmark.isNotEmpty + ? '$title, $bookmark, עמוד ${i + 1}' + : '$title, עמוד ${i + 1}'; + // Index each line from the page + for (int j = 0; j < texts.length; j++) { ... - final bookmark = await refFromPageNumber(i + 1, outline, title); - final ref = bookmark.isNotEmpty - ? '$title, $bookmark, עמוד ${i + 1}' - : '$title, עמוד ${i + 1}'; + final ref = pageRef;
138-146: Document ID collisions likely; don’t rely on microseconds for uniqueness
DateTime.now().microsecondsSinceEpochcan repeat within tight loops, causing duplicate IDs and potential index corruption.Use a stable 64-bit hash derived from book identity and segment indices.
- id: BigInt.from(DateTime.now().microsecondsSinceEpoch), + id: _docIdText(title, i),- id: BigInt.from(DateTime.now().microsecondsSinceEpoch), + id: _docIdPdf(book.path, i, j),Add these helpers in this class:
BigInt _hash64(String s) { final b = sha1.convert(utf8.encode(s)).bytes; // 20 bytes BigInt v = BigInt.zero; for (var k = 0; k < 8; k++) { v = (v << 8) | BigInt.from(b[k]); } return v; } BigInt _docIdText(String title, int lineIndex) => _hash64('txt|$title|$lineIndex'); BigInt _docIdPdf(String path, int pageIndex, int lineIndex) => _hash64('pdf|$path|$pageIndex|$lineIndex');Also applies to: 183-191
♻️ Duplicate comments (1)
installer/otzaria_full.iss (1)
3325-3327: Excludes delimiter is wrong — use semicolons, not commas.
With commas, patterns won’t match and MSIX/Appx artifacts may get bundled.Apply:
Source: "..\build\windows\x64\runner\Release\*"; \ - Excludes: "*.msix,*.msixbundle,*.appx,*.appxbundle,*.appinstaller"; \ + Excludes: "*.msix;*.msixbundle;*.appx;*.appxbundle;*.appinstaller"; \ DestDir: "{app}"; Flags: ignoreversion recursesubdirs createallsubdirs
🧹 Nitpick comments (10)
lib/library/bloc/library_bloc.dart (3)
71-77: Minor: simplify nullability if possible.
_findCategoryByPathnever returnsnullin its current logic; consider making it non-nullable and dropping?? library.
87-107: Make cycle guard identity-based to avoid false positives if Category overrides ==.Use an identity set so structurally equal categories don’t prematurely stop traversal.
Apply within this hunk:
- final visited = <Category>{}; // למניעת לולאות אינסופיות + final visited = HashSet<Category>.identity(); // למניעת לולאות אינסופיותAdd once at top of file (outside this hunk):
import 'dart:collection';Note: Path-by-title can be ambiguous if siblings share titles; if available, prefer a stable ID-based path.
109-126: Avoid exceptions for control flow in path traversal.Use
indexWhere/firstWherewithorElseinstead of try/catch; keeps it fast and clear.Apply within this hunk:
- for (final categoryName in path) { - try { - final found = current.subCategories.where((cat) => cat.title == categoryName).first; - current = found; - } catch (e) { - // אם לא מצאנו את התיקייה, נחזיר את הקרובה ביותר - return current; - } - } + for (final categoryName in path) { + final idx = current.subCategories.indexWhere((cat) => cat.title == categoryName); + if (idx < 0) break; // לא נמצא — מחזירים את הקרובה ביותר בהמשך + current = current.subCategories[idx]; + }Optional: make the return type non-nullable (
Category _findCategoryByPath(...)) and update the call site accordingly.lib/indexing/repository/indexing_repository.dart (7)
65-69: Don't await a microtask just to log; capture stack trace insteadAwaiting a microtask here adds overhead without benefit. Log directly and include stack trace.
- } catch (e) { - // Use async error handling to prevent event loop blocking - await Future.microtask(() { - debugPrint('Error adding ${book.title} to index: $e'); - }); + } catch (e, st) { + // Log error and capture stack for diagnosis + debugPrint('Error adding ${book.title} to index: $e'); + debugPrintStack(stackTrace: st);
77-78: Gate the per-book yield to reduce scheduler churnYielding after every book can be expensive on large libraries; gate it.
- await Future.delayed(Duration.zero); + if (processedBooks % 5 == 0) { + await Future.delayed(Duration.zero); + }
42-44: Persist title-keys when skipping re-index due to digest matchWhen the digest is found, you add the
${title}{type}key but don’t persist it. AddsaveIndexedBooks()in these branches to avoid losing the marker on app restart.- if (_tantivyDataProvider.booksDone.contains( - sha1.convert(utf8.encode((await book.text))).toString())) { - _tantivyDataProvider.booksDone.add("${book.title}textBook"); + if (_tantivyDataProvider.booksDone.contains( + sha1.convert(utf8.encode((await book.text))).toString())) { + _tantivyDataProvider.booksDone.add("${book.title}textBook"); + saveIndexedBooks();- if (_tantivyDataProvider.booksDone.contains( - sha1.convert(await File(book.path).readAsBytes()).toString())) { - _tantivyDataProvider.booksDone.add("${book.title}pdfBook"); + if (_tantivyDataProvider.booksDone.contains( + /* see memory-safe hashing suggestion below */ digest)) { + _tantivyDataProvider.booksDone.add("${book.title}pdfBook"); + saveIndexedBooks();Also applies to: 53-55
101-106: Skip the first yield; start after some useful workYielding at
i == 0isn’t necessary.- if (i % 100 == 0) { + if (i > 0 && i % 100 == 0) { await Future.delayed(Duration.zero); }
173-177: Same here: avoid yielding on the very first line of each pageStart yielding after actual work to reduce overhead.
- if (j % 50 == 0) { + if (j > 0 && j % 50 == 0) { await Future.delayed(Duration.zero); }
214-221: Expose current indexing state as a getter propertyTiny API nit: prefer a getter for
isIndexingto align with Dart idioms.- bool isIndexing() { - return _tantivyDataProvider.isIndexing.value; - } + bool get isIndexing => _tantivyDataProvider.isIndexing.value;
36-61: Sanity-check: are digest entries ever added tobooksDone?You check for content digests in
booksDonebut never add them in this file. If nothing else adds them, these checks will always miss. Confirm the intended contract; if needed, add digest entries upon successful indexing for both text and PDF.If you want, I can wire digest writes into
_indexTextBook/_indexPdfBookand persist viasaveIndexedBooks().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.gitignore(1 hunks)installer/otzaria.iss(2 hunks)installer/otzaria_full.iss(2 hunks)lib/indexing/repository/indexing_repository.dart(3 hunks)lib/library/bloc/library_bloc.dart(2 hunks)lib/library/view/library_browser.dart(9 hunks)lib/navigation/main_window_screen.dart(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- installer/otzaria.iss
- lib/navigation/main_window_screen.dart
- lib/library/view/library_browser.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_android
- GitHub Check: build_linux
- GitHub Check: build_windows
- GitHub Check: build_macos
🔇 Additional comments (6)
installer/otzaria_full.iss (1)
5-5: Version bump LGTM — installer/otzaria_full.iss and installer/otzaria.iss both use “0.9.47”. Confirm any other .iss scripts use the same.lib/library/bloc/library_bloc.dart (3)
51-53: Nice UX: preserving the user’s location across refresh.Capturing the current category path before reload is the right move.
59-61: Refresh source of truth looks correct.Re-pointing the repository to the freshly scanned filesystem library before awaiting it is sound.
63-69: Replace print withdev.log; no await needed forreopenIndex
reopenIndexis a synchronousvoidmethod, so awaiting it is invalid. Only swap out thediff --git a/lib/library/bloc/library_bloc.dart b/lib/library/bloc/library_bloc.dart index abc1234..def5678 100644 --- a/lib/library/bloc/library_bloc.dart +++ b/lib/library/bloc/library_bloc.dart @@ -1,5 +1,6 @@ +import 'dart:developer' as dev; import 'package:bloc/bloc.dart'; import 'package:your_app/data_providers/tantivy_data_provider.dart'; @@ -63,7 +64,7 @@ class LibraryBloc extends Bloc<LibraryEvent, LibraryState> { try { - TantivyDataProvider.instance.reopenIndex(); + TantivyDataProvider.instance.reopenIndex(); } catch (e) { - print('Warning: Could not reopen search index: $e'); + dev.log('Could not reopen search index', error: e, name: 'LibraryBloc'); }Likely an incorrect or invalid review comment.
lib/indexing/repository/indexing_repository.dart (2)
71-75: Good: progress still reported after errors and yielding back to the loopKeeping UX responsive while preserving progress is correct.
198-201: Propagate cancellation to isolate-based indexer
NoIndexingIsolateService.cancelIndexingcalls were found in the codebase—if you’re running indexing in an isolate, ensurecancelIndexing()also signals that service.
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores