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 (5)
lib/empty_library/bloc/empty_library_bloc.dart (5)
218-257: Zip‑Slip vulnerability when extracting entries; sanitize archive paths and constrain to destination.
file.namecan contain../or absolute paths allowing writes outsidelibraryPath. Validate and normalize paths before writing.Apply this diff (also add the import in the imports section):
+import 'package:path/path.dart' as p- for (final file in archive.files) { + for (final file in archive.files) { if (!_isCancelling) { final filename = file.name; - final filePath = '$libraryPath/$filename'; + final resolvedPath = p.normalize(p.join(libraryPath, filename)); + if (!p.isWithin(libraryPath, resolvedPath)) { + throw Exception('Invalid path in archive (zip‑slip): $filename'); + } add(DownloadProgressUpdated( progress: extractedFiles / totalFiles, currentOperation: 'מחלץ: $filename', downloadedMB: state.downloadedMB, downloadSpeed: state.downloadSpeed)); try { if (file.isFile) { - final outputFile = File(filePath); + final outputFile = File(resolvedPath); await outputFile.parent.create(recursive: true); - final outputStream = OutputFileStream(outputFile.path); - file.writeContent(outputStream); - outputStream.close(); + final outputStream = OutputFileStream(outputFile.path); + try { + file.writeContent(outputStream); + } finally { + outputStream.close(); + } } else { - await Directory(filePath).create(recursive: true); + await Directory(resolvedPath).create(recursive: true); } extractedFiles++; } catch (e) { print('Error extracting $filename: $e'); throw Exception('שגיאה בחילוץ הקובץ $filename: $e'); } } } - inputStream.close(); + inputStream.close();
260-272: Also sanitize paths when using flutter_archive; skip unsafe entries.Guard
zipEntry.namethe same way and skip items outsidelibraryPath.Apply this diff:
- onExtracting: (zipEntry, progress) { - add(DownloadProgressUpdated( + onExtracting: (zipEntry, progress) { + final resolvedPath = p.normalize(p.join(libraryPath, zipEntry.name)); + if (!p.isWithin(libraryPath, resolvedPath)) { + return flutter_archive.ZipFileOperation.skipItem; + } + add(DownloadProgressUpdated( progress: progress, currentOperation: 'מחלץ: ${zipEntry.name}', downloadedMB: state.downloadedMB, downloadSpeed: state.downloadSpeed)); return flutter_archive.ZipFileOperation.includeItem; });
160-171: HTTP client not closed; no timeouts; brittle status check.
http.Client()is never closed (socket leak) and the request has no timeout. Also, accept any 2xx, not only 200.Apply this diff:
- final request = http.Request( - 'GET', - Uri.parse( - 'https://github.com/zevisvei/otzaria-library/releases/download/latest/otzaria_latest.zip'), - ); - final response = await http.Client().send(request); + final client = http.Client(); + final request = http.Request( + 'GET', + Uri.parse('https://github.com/zevisvei/otzaria-library/releases/download/latest/otzaria_latest.zip'), + )..headers.addAll({ + 'Accept': 'application/octet-stream', + }); + final response = await client.send(request).timeout(const Duration(minutes: 5)); - if (response.statusCode != 200) { + if (response.statusCode < 200 || response.statusCode >= 300) { throw Exception('Failed to start download: ${response.statusCode}'); }And ensure cleanup on both success and error:
onDone: () async { if (_isCancelling) return; + client.close(); await _fileSink?.flush(); await _fileSink?.close(); _fileSink = null;- onError: (error) { - emit(EmptyLibraryError(errorMessage: 'שגיאה בהורדה: $error')); - }, + onError: (error) async { + try { + await _fileSink?.flush(); + } catch (_) {} + await _fileSink?.close(); + _fileSink = null; + _speedTimer?.cancel(); + client.close(); + await _cleanupTempFile(); + emit(EmptyLibraryError(errorMessage: 'שגיאה בהורדה: $error')); + },Also applies to: 290-293
221-257: Close streams with try/finally to avoid handle leaks if exceptions occur mid‑extract.
inputStream.close()andoutputStream.close()should be infinallyblocks.Apply this diff:
- final inputStream = InputFileStream(_tempFile!.path); - final archive = extractor.decodeBuffer(inputStream); + final inputStream = InputFileStream(_tempFile!.path); + try { + final archive = extractor.decodeBuffer(inputStream); ... - final outputStream = OutputFileStream(outputFile.path); - try { - file.writeContent(outputStream); - } finally { - outputStream.close(); - } + final outputStream = OutputFileStream(outputFile.path); + try { + file.writeContent(outputStream); + } finally { + outputStream.close(); + } ... - inputStream.close(); + } finally { + inputStream.close(); + }
174-176: Ensure temp ZIP is always cleaned up via finally.If extraction throws,
_cleanupTempFile()may not run. Wrap extraction phase in try/finally.Apply this diff:
- try { + try { if (!await _tempFile!.exists()) { throw Exception('קובץ הספרייה הזמני לא נמצא'); } ... - await _cleanupTempFile(); - - emit(EmptyLibraryDownloaded()); - } catch (e) { + emit(EmptyLibraryDownloaded()); + } catch (e) { emit(EmptyLibraryError(errorMessage: 'שגיאה בחילוץ: $e')); + } finally { + await _cleanupTempFile(); }Also applies to: 195-207, 283-288
♻️ Duplicate comments (3)
lib/navigation/calendar_cubit.dart (2)
891-894: Potential null pointer dereference in _calculateChatzosLayla.The function assumes
getChatzos()returns a non-null value, but the return type indicates it could be null.
897-901: Potential null pointer dereference in _calculateSunsetRT.Similar to the previous issue,
getSunset()could return null..github/workflows/flutter.yml (1)
13-16: Gate releases to main/dev only (feature-branch pushes will publish releases).These extra push branches will trigger the create_release job for feature branches. Remove them or guard the job by branch. This was flagged earlier and is still applicable.
Option A — restrict on.push:
push: branches: - main - dev - dev_dev2 - - n-search2 - - ns2new - - n-search2.1Option B — gate the job (recommended even if Option A is applied):
jobs: create_release: - if: github.event_name == 'push' + if: github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev')
🧹 Nitpick comments (27)
.github/workflows/release-webhook.yml (4)
1-1: Set least‑privilege permissions.Explicitly scope the GITHUB_TOKEN to read‑only for contents.
name: Send Release Webhook +permissions: + contents: read
10-12: Add a timeout to avoid hung runs.Prevent indefinite executions if the script blocks on network I/O.
send_webhook: runs-on: ubuntu-latest + timeout-minutes: 10
15-21: Pin GitHub Actions to immutable SHAs.actions/checkout@v4 and actions/setup-python@v5 should be pinned to commit SHAs to mitigate supply‑chain risk. Keep the tag as a comment for readability.
Would you like me to propose the current SHAs for these actions?
26-34: Coalesce env vars for both triggers.Make the env work for release and workflow_dispatch runs.
- name: Run webhook script env: - RELEASE_TAG: ${{ github.event.release.tag_name }} - RELEASE_NAME: ${{ github.event.release.name }} - RELEASE_BODY: ${{ github.event.release.body }} - RELEASE_URL: ${{ github.event.release.html_url }} + RELEASE_TAG: ${{ github.event.release.tag_name || inputs.release_tag }} + RELEASE_NAME: ${{ github.event.release.name || inputs.release_name }} + RELEASE_BODY: ${{ github.event.release.body || inputs.release_body }} + RELEASE_URL: ${{ github.event.release.html_url || inputs.release_url }} USER_NAME: ${{ secrets.USER_NAME }} PASSWORD: ${{ secrets.PASSWORD }} TOKEN_YEMOT: ${{ secrets.TOKEN_YEMOT }} run: python webhooks/main.pylib/bookmarks/bookmark_screen.dart (8)
22-47: Tighten lifecycle: normalize query and guard focus.
- Normalize query once and avoid redundant rebuilds.
- Guard requestFocus with mounted to prevent edge-case callbacks after dispose.
Apply this diff:
@override void initState() { super.initState(); - _searchController.addListener(() { - setState(() { - _searchQuery = _searchController.text; - }); - }); + _searchController.addListener(() { + final q = _searchController.text.trim().toLowerCase(); + if (q != _searchQuery) { + setState(() => _searchQuery = q); + } + }); // Auto-focus the search field when the screen opens WidgetsBinding.instance.addPostFrameCallback((_) { - _searchFocusNode.requestFocus(); + if (!mounted) return; + _searchFocusNode.requestFocus(); }); }
74-77: Consider showing the search UI even when there are no bookmarks.Helps users understand the new search affordance and prevents UI “jumping” when the first bookmark is added.
78-83: Avoid O(n^2) index lookups; precompute filtered entries with original indices.Current use of indexOf per row is quadratic and can misfire if the list updates mid‑interaction.
Apply this diff:
- // Filter bookmarks based on search query - final filteredBookmarks = _searchQuery.isEmpty - ? state.bookmarks - : state.bookmarks.where((bookmark) => - bookmark.ref.toLowerCase().contains(_searchQuery.toLowerCase())).toList(); + // Filter with original indices (prevents O(n^2) indexOf in the builder) + final filteredEntries = _searchQuery.isEmpty + ? state.bookmarks.asMap().entries.toList() + : state.bookmarks + .asMap() + .entries + .where((e) => e.value.ref.toLowerCase().contains(_searchQuery)) + .toList();
84-111: Search field UX: add RTL alignment and “search” IME action.Improves Hebrew input and accessibility.
Apply this diff:
child: TextField( controller: _searchController, focusNode: _searchFocusNode, + textAlign: TextAlign.right, + textDirection: TextDirection.rtl, + textInputAction: TextInputAction.search, decoration: InputDecoration( hintText: 'חפש בסימניות...', prefixIcon: const Icon(Icons.search),
95-102: Remove redundant setState; controller listener already updates state.Prevents double rebuilds on clear.
Apply this diff:
- onPressed: () { - _searchController.clear(); - setState(() { - _searchQuery = ''; - }); - }, + onPressed: () => _searchController.clear(),
112-149: Use precomputed entries; add safety check and stable keys.
- Switch to filteredEntries from above.
- Guard against stale indices.
- Provide a stable Key per row to help ListView diffing.
Apply this diff:
- child: filteredBookmarks.isEmpty + child: filteredEntries.isEmpty ? const Center(child: Text('לא נמצאו תוצאות')) : ListView.builder( - itemCount: filteredBookmarks.length, + itemCount: filteredEntries.length, itemBuilder: (context, index) { - final bookmark = filteredBookmarks[index]; - final originalIndex = state.bookmarks.indexOf(bookmark); + final entry = filteredEntries[index]; + final originalIndex = entry.key; + final bookmark = entry.value; return ListTile( + key: ValueKey('${bookmark.ref}|${bookmark.index}'), selected: false, leading: bookmark.book is PdfBook ? const Icon(Icons.picture_as_pdf) : null, title: Text(bookmark.ref), onTap: () => _openBook( context, bookmark.book, bookmark.index, bookmark.commentatorsToShow), trailing: IconButton( - icon: const Icon( - Icons.delete_forever, - ), + icon: const Icon(Icons.delete_forever), onPressed: () { - context - .read<BookmarkBloc>() - .removeBookmark(originalIndex); + if (originalIndex < 0 || originalIndex >= state.bookmarks.length) return; + context.read<BookmarkBloc>().removeBookmark(originalIndex);
137-139: Prefer deleting by ID/unique key instead of index.Index-based deletes are brittle under concurrent updates/reordering. If Bookmark exposes an id/historyKey, emit a removeById event instead.
Can you confirm whether Bookmark has a unique identifier (e.g., id or historyKey) we can route through the BLoC? I can draft the event/state changes if so.
150-166: Tiny cleanup and UX consideration.
- Remove redundant const in Duration.
- Consider a confirmation dialog before mass deletion.
Apply this diff:
const SnackBar( content: Text('כל הסימניות נמחקו'), - duration: const Duration(milliseconds: 350), + duration: Duration(milliseconds: 350), ),lib/navigation/calendar_cubit.dart (1)
1044-1044: Useconst Durationfor better performance.Duration constructors can be made constant for compile-time optimization.
Apply this diff to improve performance:
- return sunset.subtract(Duration(minutes: minutesBefore)); + return sunset.subtract(Duration(minutes: minutesBefore));- return sunset.add(const Duration(minutes: 34)); + return sunset.add(const Duration(minutes: 34));- return sunset.add(const Duration(minutes: 38)); + return sunset.add(const Duration(minutes: 38));Note: Line 1044 cannot use const because
minutesBeforeis a variable, but lines 1052 and 1060 are already using const correctly.Also applies to: 1052-1052, 1060-1060
lib/history/history_screen.dart (6)
160-164: Initialize the tab before attaching it to TabsBloc to avoid blank/flicker racesAdding the tab before setting its query/options can briefly render an empty tab. Initialize first, then add the tab, then dispatch the search.
Apply:
- final tabsBloc = context.read<TabsBloc>(); - // Always create a new search tab instead of reusing existing one - final searchTab = SearchingTab('חיפוש', null); - tabsBloc.add(AddTab(searchTab)); + final tabsBloc = context.read<TabsBloc>(); + // Always create a new search tab instead of reusing existing one + final searchTab = SearchingTab('חיפוש', null); @@ - // Trigger search - searchTab.searchBloc.add(UpdateSearchQuery( + // Attach after initialization to avoid flicker + tabsBloc.add(AddTab(searchTab)); + // Trigger search + searchTab.searchBloc.add(UpdateSearchQuery(Also applies to: 177-186
167-176: Avoid untyped{}defaults inaddAll— risk of type inference to Map<dynamic,dynamic>Use null checks with non-null assertion to keep collection types intact.
Apply:
- searchTab.searchOptions.clear(); - searchTab.searchOptions - .addAll(historyItem.searchOptions ?? {}); + searchTab.searchOptions..clear(); + if (historyItem.searchOptions != null) { + searchTab.searchOptions.addAll(historyItem.searchOptions!); + } @@ - searchTab.alternativeWords.clear(); - searchTab.alternativeWords - .addAll(historyItem.alternativeWords ?? {}); + searchTab.alternativeWords..clear(); + if (historyItem.alternativeWords != null) { + searchTab.alternativeWords.addAll(historyItem.alternativeWords!); + } @@ - searchTab.spacingValues.clear(); - searchTab.spacingValues - .addAll(historyItem.spacingValues ?? {}); + searchTab.spacingValues..clear(); + if (historyItem.spacingValues != null) { + searchTab.spacingValues.addAll(historyItem.spacingValues!); + }
41-43: Guard autofocus withmountedinside post-frame callbackPrevents requesting focus after disposal during quick route changes.
Apply:
- WidgetsBinding.instance.addPostFrameCallback((_) { - _searchFocusNode.requestFocus(); - }); + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted) return; + _searchFocusNode.requestFocus(); + });
131-136: Remove redundantsetState— listener already updates_searchQueryon clearAvoids double rebuild.
Apply:
- onPressed: () { - _searchController.clear(); - setState(() { - _searchQuery = ''; - }); - }, + onPressed: () { + _searchController.clear(); + },
59-61: DRY: extractopenLeftPaneexpression onceImproves readability; avoids evaluating settings twice.
Apply:
- final tab = book is PdfBook + final openLeftPane = + (Settings.getValue<bool>('key-pin-sidebar') ?? false) || + (Settings.getValue<bool>('key-default-sidebar-open') ?? false); + final tab = book is PdfBook ? PdfBookTab( book: book, pageNumber: index, - openLeftPane: (Settings.getValue<bool>('key-pin-sidebar') ?? - false) || - (Settings.getValue<bool>('key-default-sidebar-open') ?? false), + openLeftPane: openLeftPane, ) : TextBookTab( book: book as TextBook, index: index, commentators: commentators, - openLeftPane: (Settings.getValue<bool>('key-pin-sidebar') ?? - false) || - (Settings.getValue<bool>('key-default-sidebar-open') ?? false), + openLeftPane: openLeftPane, );Also applies to: 67-70
123-145: UX: add keyboard actions for the search fieldOptional: add
textInputAction: TextInputAction.searchandonSubmitted: (_) => _searchFocusNode.unfocus()to improve ergonomics.installer/otzaria_full.iss (1)
3308-3308: Potential data loss: deleting default.isar on every install.
If default.isar contains user data, this wipes it during full installs. If the intent is only to remove a stale file, restrict to files to avoid directory semantics.Consider:
-Type: filesandordirs; Name: "{app}\default.isar"; +Type: files; Name: "{app}\default.isar";Confirm whether this delete is truly desired on upgrade paths for the “full” build.
.github/workflows/flutter.yml (3)
34-66: Harden ISCC.exe discovery (handle per‑user installs).Winget can install Inno Setup under LOCALAPPDATA; the current search only checks Program Files. Add a LOCALAPPDATA fallback before failing.
- $iscc = (Get-ChildItem "C:\Program Files*\Inno Setup*\ISCC.exe" -Recurse -ErrorAction SilentlyContinue | - Select-Object -First 1 -ExpandProperty FullName) - if (-not $iscc) { throw "ISCC.exe not found after installation" } + $candidates = @( + "C:\Program Files*\Inno Setup*\ISCC.exe", + "$env:LOCALAPPDATA\Programs\Inno Setup*\ISCC.exe" + ) + $iscc = $null + foreach ($pat in $candidates) { + $hit = Get-ChildItem $pat -Recurse -ErrorAction SilentlyContinue | Select-Object -First 1 -ExpandProperty FullName + if ($hit) { $iscc = $hit; break } + } + if (-not $iscc) { throw "ISCC.exe not found after installation (checked Program Files and LOCALAPPDATA)" }
82-85: Make MSIX build non‑blocking and align upload behavior.msix:create can fail if tooling/cert isn’t present; let the pipeline proceed and have upload ignore missing files.
Within this step:
- - name: Build MSIX package - shell: pwsh - run: | - dart run msix:create --install-certificate false + - name: Build MSIX package + shell: pwsh + continue-on-error: true + run: | + dart run msix:create --install-certificate falseAnd in “Upload Windows MSIX” (outside this hunk):
- - name: Upload Windows MSIX + - name: Upload Windows MSIX uses: actions/upload-artifact@v4 with: name: otzaria.msix - path: build/windows/x64/runner/release/*.msix + path: build/windows/x64/runner/release/*.msix + if-no-files-found: ignore
372-377: Standardize PR tag prefix (consistency with release tags).PR prerelease tags drop the leading “v”. Consider using v$VERSION-pr-… or pr/v$VERSION-… for consistent sorting and tooling that expects a v‑prefixed semver.
- echo "tag=$VERSION-pr-${{ github.event.number }}-${{ github.run_number }}" >> $GITHUB_OUTPUT + echo "tag=v$VERSION-pr-${{ github.event.number }}-${{ github.run_number }}" >> $GITHUB_OUTPUT # or +# echo "tag=pr/v$VERSION-${{ github.event.number }}-${{ github.run_number }}" >> $GITHUB_OUTPUTlib/empty_library/bloc/empty_library_bloc.dart (4)
134-143: Use a system temp directory for transient downloads, not Documents.Prefer
getTemporaryDirectory()for_tempFileto avoid cluttering user documents and to get auto‑cleanup on some platforms.Apply this diff:
- final tempDir = await getApplicationDocumentsDirectory(); + final tempDir = await getTemporaryDirectory();
145-158: Nit: variable name implies bytes but holds MB.
_lastDownloadedBytestracks MB. Rename to_lastDownloadedMBto avoid confusion.
171-191: Optional: guard by expected content type and a max size.Before starting extraction, validate
response.headers['content-type']isapplication/ziporapplication/octet-stream, and fail ifcontentLengthexceeds a sane cap to avoid zip bombs.
321-331: State naming: emitting “Downloaded” after directory pick is misleading.Consider a dedicated
DirectorySelectedstate to avoid UI reacting as if content was actually downloaded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/flutter.yml(3 hunks).github/workflows/release-webhook.yml(1 hunks).gitignore(1 hunks)installer/otzaria.iss(2 hunks)installer/otzaria_full.iss(2 hunks)lib/bookmarks/bookmark_screen.dart(2 hunks)lib/empty_library/bloc/empty_library_bloc.dart(1 hunks)lib/history/history_screen.dart(3 hunks)lib/navigation/calendar_cubit.dart(1 hunks)lib/navigation/calendar_widget.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .gitignore
- installer/otzaria.iss
- lib/navigation/calendar_widget.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_macos
- GitHub Check: build_android
- GitHub Check: build_linux
- GitHub Check: build_windows
🔇 Additional comments (12)
.github/workflows/release-webhook.yml (2)
25-35: LGTM overall.The workflow wiring is clear and minimal. With the above adjustments, it’ll be robust for both release and manual runs.
26-35: Secrets hygiene — confirm secrets exist & are not logged.webhooks/main.py is present and reads RELEASE_TAG/RELEASE_NAME/RELEASE_BODY/RELEASE_URL and USER_NAME/PASSWORD/TOKEN_YEMOT (envs on lines ~17–24) and instantiates MitmachimClient with username/password (line ~38). Ensure USER_NAME, PASSWORD, TOKEN_YEMOT are set as repo/org secrets and that webhooks/main.py never logs/prints these values (search for print()/logging.*/f-strings containing them). Verify the workflow path (webhooks/main.py) is correct.
lib/bookmarks/bookmark_screen.dart (1)
15-20: Stateful migration looks good; no public API break.Constructor remains const; imports and call sites should be unaffected.
Please confirm that any tests relying on StatelessWidget-specific behavior (e.g., const subtree optimizations) still pass.
lib/history/history_screen.dart (3)
84-86: Verify: Why is.docxhandled underPdfBook?If DOCX entries aren’t modeled as
PdfBook, this branch is unreachable or misclassified. Consider basing the icon purely on file extension or actual type.
185-191: Confirm pop-after-navigation behavior
NavigateToScreen(Screen.search)via bloc plusNavigator.pop()can unintentionally pop a page if this isn’t inside a dialog. Please confirm this screen is only ever shown modally whencanPop == true.
159-193: Sanity-check passed — SearchingTab constructor/fields align with history model.
SearchingTab accepts a nullable searchText and exposes queryController, searchBloc, searchOptions (Map<String, Map<String, bool>>), alternativeWords (Map<int, List>) and spacingValues (Map<String, String>); the Bookmark/history item defines matching nullable fields. Verified in lib/tabs/models/searching_tab.dart, lib/bookmarks/models/bookmark.dart and lib/history/bloc/history_bloc.dart.installer/otzaria_full.iss (2)
5-5: Version bump verified — consistent across installers and CI artifacts.
installer/otzaria.iss:5 and installer/otzaria_full.iss:5 define MyAppVersion "0.9.53"; both OutputBaseFilename lines use otzaria-{#MyAppVersion}-windows(-full) (line 27); version.json and pubspec.yaml contain 0.9.53; update_version.ps1 updates both; .github/workflows/flutter.yml uses installer/otzaria-*-windows.exe (wildcard).
3325-3328: Incorrect — Excludes uses commas, not semicolons. Inno Setup's Excludes parameter expects comma-separated patterns; the current entries already use commas, so switching to semicolons would be incorrect. (documentation.help)Files to ignore: installer/otzaria_full.iss (lines 3325–3327) and installer/otzaria.iss (line 52) — Excludes currently: ".msix,.msixbundle,.appx,.appxbundle,*.appinstaller".
If you need stricter filtering, add path-aware masks (for example "\release*" or "\Release*") or additional patterns, but do not replace the commas with semicolons.
Likely an incorrect or invalid review comment.
.github/workflows/flutter.yml (4)
23-23: Good pin: windows-2022 is stable vs windows-latest drift.
26-27: Checkout v4 is current and fine.
68-68: pwsh for Windows build = consistent with later steps.
87-91: ISCC via explicit path is correct; keep quoting.
| on: | ||
| release: | ||
| types: [published] | ||
|
|
||
| workflow_dispatch: # מאפשר הפעלה ידנית | ||
|
|
There was a problem hiding this comment.
Manual trigger has no release context — envs will be empty.
When run via workflow_dispatch, github.event.release.* is undefined, so RELEASE_* envs become empty and the script may misbehave. Add inputs and coalesce with release event fields.
Apply this diff:
on:
release:
types: [published]
- workflow_dispatch: # מאפשר הפעלה ידנית
+ workflow_dispatch: # מאפשר הפעלה ידנית
+ inputs:
+ release_tag:
+ description: "Tag (e.g., v1.2.3)"
+ required: true
+ release_name:
+ description: "Release name"
+ required: false
+ release_body:
+ description: "Release notes (optional)"
+ required: false
+ release_url:
+ description: "Release URL (optional)"
+ required: falseAnd coalesce env vars (see lines 26-34 diff in a separate comment).
📝 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.
| on: | |
| release: | |
| types: [published] | |
| workflow_dispatch: # מאפשר הפעלה ידנית | |
| on: | |
| release: | |
| types: [published] | |
| workflow_dispatch: # מאפשר הפעלה ידנית | |
| inputs: | |
| release_tag: | |
| description: "Tag (e.g., v1.2.3)" | |
| required: true | |
| release_name: | |
| description: "Release name" | |
| required: false | |
| release_body: | |
| description: "Release notes (optional)" | |
| required: false | |
| release_url: | |
| description: "Release URL (optional)" | |
| required: false |
🤖 Prompt for AI Agents
In .github/workflows/release-webhook.yml around lines 3 to 8, the
workflow_dispatch manual trigger runs without release context so
github.event.release.* is undefined and RELEASE_* envs become empty; add inputs
to the workflow_dispatch for the needed release fields (tag_name, name, body,
draft, prerelease, created_at, published_at, author_login, etc.) and then
coalesce environment variables (lines ~26-34) to use the provided inputs first
and fall back to github.event.release.* when inputs are empty (e.g., set
RELEASE_TAG: ${{ inputs.tag_name || github.event.release.tag_name }}), ensuring
scripts receive sensible values when manually triggered.
| - name: Install dependencies | ||
| run: pip install bs4 requests pyluach | ||
|
|
There was a problem hiding this comment.
Unpinned Python deps in CI.
Installing bs4/requests/pyluach without pinning is a supply‑chain risk and harms reproducibility. Prefer a hashed requirements file.
Suggested change (you’ll need to add webhooks/requirements.txt with pinned versions and --hash entries):
- - name: Install dependencies
- run: pip install bs4 requests pyluach
+ - name: Install dependencies
+ run: |
+ python -m pip install --upgrade pip
+ python -m pip install --require-hashes -r webhooks/requirements.txtMinor: install beautifulsoup4 instead of the bs4 shim for clarity.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/release-webhook.yml lines 22-24: the workflow installs
unpinned Python packages which is a supply-chain and reproducibility risk;
replace the direct pip install with pip install -r webhooks/requirements.txt
--require-hashes and create webhooks/requirements.txt containing pinned versions
with their corresponding --hash entries (use beautifulsoup4 instead of the bs4
shim and include requests and pyluach) so CI installs deterministic, verified
packages.
| 'GET', | ||
| Uri.parse( | ||
| 'https://github.com/Sivan22/otzaria-library/releases/download/latest/otzaria_latest.zip'), | ||
| 'https://github.com/zevisvei/otzaria-library/releases/download/latest/otzaria_latest.zip'), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Download origin changed to a different GitHub account; pin and verify artifacts instead of using “latest”.
Switching to github.com/zevisvei/... is a supply‑chain risk. Avoid mutable latest URLs; pin to a specific tag/commit and verify checksums before extraction. Add fallback/mirror or a server‑side redirect you control.
Would you confirm ownership/transfer of the new repo and whether a signed checksum (e.g., SHA‑256) is published per release so we can validate the ZIP before extraction?
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repository for 'otzaria_latest.zip', 'zevisvei' and related strings..."
rg -n --hidden -S "otzaria_latest.zip|zevisvei|otzaria-library" -C2 || true
echo
echo "Showing lib/empty_library/bloc/empty_library_bloc.dart lines 150-170 if present..."
if [ -f "lib/empty_library/bloc/empty_library_bloc.dart" ]; then
nl -ba lib/empty_library/bloc/empty_library_bloc.dart | sed -n '150,170p'
else
echo "File not found: lib/empty_library/bloc/empty_library_bloc.dart"
echo "Searching for any file that contains 'otzaria' under lib/..."
rg -n --hidden -S "otzaria" lib -C2 || true
echo "Listing lib/empty_library directory (if exists)..."
ls -la lib/empty_library || true
fiLength of output: 3199
Stop downloading 'latest' from an external GitHub account — pin releases and verify checksums.
Found a mutable release URL pointing to a different owner (supply‑chain risk). Pin to a specific tag/commit, validate the ZIP (SHA‑256 or GPG-signed checksum) before extraction, and add a server-controlled mirror/fallback.
- lib/empty_library/bloc/empty_library_bloc.dart:163 — 'https://github.com/zevisvei/otzaria-library/releases/download/latest/otzaria_latest.zip'
- lib/library/view/library_browser.dart:155 — githubOwner: "zevisvei", repositoryName: "otzaria-library", branch: "main"
- README.md:134,140,150 — links still reference github.com/Sivan22/otzaria-library/releases (inconsistent)
Confirm ownership/transfer of zevisvei/otzaria-library and whether signed checksums (SHA‑256 or GPG) are published per release so we can validate before extraction.
🤖 Prompt for AI Agents
lib/empty_library/bloc/empty_library_bloc.dart line 163: the code downloads a
mutable "latest" ZIP from zevisvei's GitHub which is a supply‑chain risk; change
the URL to a pinned release asset (use a specific tag or commit SHA in the
release URL), add code to fetch and verify a published checksum (preferably
SHA‑256 or GPG signature) before extracting, and add a configurable
server-controlled mirror/fallback URL to use if verification fails; also update
lib/library/view/library_browser.dart and README links to use the same pinned
owner/repo/branch values or your verified mirror and verify ownership/transfer
of zevisvei/otzaria-library and that signed checksums are published for the
pinned release.
| // Filter history based on search query | ||
| final filteredHistory = _searchQuery.isEmpty | ||
| ? state.history | ||
| : state.history.where((item) => | ||
| item.ref.toLowerCase().contains(_searchQuery.toLowerCase())).toList(); | ||
|
|
There was a problem hiding this comment.
Delete-by-index may remove the wrong item when duplicates exist; avoid O(n²) rendering
state.history.indexOf(historyItem) picks the first equal item; duplicates will delete the wrong row, and doing this per row is O(n²).
Refactor to carry original indices via asMap():
- // Filter history based on search query
- final filteredHistory = _searchQuery.isEmpty
- ? state.history
- : state.history.where((item) =>
- item.ref.toLowerCase().contains(_searchQuery.toLowerCase())).toList();
+ // Filter history while preserving original indices
+ final q = _searchQuery.trim().toLowerCase();
+ final filteredEntries = (_searchQuery.isEmpty
+ ? state.history.asMap().entries
+ : state.history
+ .asMap()
+ .entries
+ .where((e) => e.value.ref.toLowerCase().contains(q)))
+ .toList();
@@
- child: filteredHistory.isEmpty
+ child: filteredEntries.isEmpty
@@
- itemCount: filteredHistory.length,
+ itemCount: filteredEntries.length,
@@
- final historyItem = filteredHistory[index];
- final originalIndex = state.history.indexOf(historyItem);
+ final entry = filteredEntries[index];
+ final historyItem = entry.value;
+ final originalIndex = entry.key;Also applies to: 147-154
🤖 Prompt for AI Agents
In lib/history/history_screen.dart around lines 112-117 (also apply same fix to
147-154): the filtered list and subsequent delete action rely on
state.history.indexOf(historyItem), which returns the first equal item (wrong
with duplicates) and causes O(n²) behavior; instead, produce the filtered list
with original indices (use state.history.asMap().entries and filter by value to
build a List of MapEntry<int, HistoryItem> or a tuple of (index,item)), use the
original index when rendering rows and when dispatching delete actions, and
avoid calling indexOf per row so deletion targets the correct item and rendering
is O(n).
| if (historyItem.isSearch) { | ||
| final tabsBloc = context.read<TabsBloc>(); | ||
| // Always create a new search tab instead of reusing existing one | ||
| final searchTab = SearchingTab('חיפוש', null); | ||
| tabsBloc.add(AddTab(searchTab)); | ||
|
|
||
| // Restore search query and options | ||
| searchTab.queryController.text = historyItem.book.title; | ||
| searchTab.searchOptions.clear(); |
There was a problem hiding this comment.
Fix crash and wrong query restoration for search history items
Accessing historyItem.book.title when isSearch == true can be null and crash; also it restores the book title instead of the saved search query. Use the history entry’s textual ref for the search box.
Apply:
- // Restore search query and options
- searchTab.queryController.text = historyItem.book.title;
+ // Restore search query and options
+ // For search history entries, use the saved ref/query text
+ searchTab.queryController.text = historyItem.ref;📝 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 (historyItem.isSearch) { | |
| final tabsBloc = context.read<TabsBloc>(); | |
| // Always create a new search tab instead of reusing existing one | |
| final searchTab = SearchingTab('חיפוש', null); | |
| tabsBloc.add(AddTab(searchTab)); | |
| // Restore search query and options | |
| searchTab.queryController.text = historyItem.book.title; | |
| searchTab.searchOptions.clear(); | |
| if (historyItem.isSearch) { | |
| final tabsBloc = context.read<TabsBloc>(); | |
| // Always create a new search tab instead of reusing existing one | |
| final searchTab = SearchingTab('חיפוש', null); | |
| tabsBloc.add(AddTab(searchTab)); | |
| // Restore search query and options | |
| // For search history entries, use the saved ref/query text | |
| searchTab.queryController.text = historyItem.ref; | |
| searchTab.searchOptions.clear(); |
🤖 Prompt for AI Agents
In lib/history/history_screen.dart around lines 159 to 167, the code reads
historyItem.book.title for search history items which can be null and also
restores the book title instead of the saved search query; replace that access
with the history entry’s textual reference (e.g. historyItem.textualRef) and
guard against null by setting searchTab.queryController.text =
historyItem.textualRef ?? '' so the query restores correctly and no null access
occurs; leave the search options restoration as-is or map from historyItem if a
saved options field exists.
2 participants
Summary by CodeRabbit