Helper scripts - #318
Merged
Merged
Conversation
Contributor
WalkthroughThis change restructures the offline data acquisition workflow by moving Ollama model pulling from a dedicated script into a Taskfile task, introducing a new Kiwix download task, adding file version retention logic to the file download script, and adding clarifying documentation comments to multiple utility scripts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/get-offline-data-file.sh (1)
60-74:⚠️ Potential issue | 🟡 MinorRetention is skipped when the latest file is already cached.
The early
return 0at lines 60–63 short-circuits before the new pruning block at 69–74, so old versions are only pruned during runs that actually download a new file. If the upstream version hasn't changed (steady state) or stale files predate this feature, they'll accumulate indefinitely. Consider hoisting the prune step so it runs on every successful invocation, regardless of whether a download occurred.♻️ Suggested fix: prune on every successful run
- # Check if the file already exists locally - if [[ -f "$LATEST_FILE" ]]; then - echo "File '$LATEST_FILE' already exists locally. No download needed." - return 0 - fi - - # Download the latest file - echo "Downloading the latest file: $LATEST_FILE..." - if wget -q --show-progress "${url}${LATEST_FILE}"; then - echo "File '$LATEST_FILE' successfully downloaded to $(pwd)." - local OLD_FILES - OLD_FILES=$(for f in *; do [[ "$f" =~ ^${pattern}$ ]] && echo "$f"; done | sort -r | tail -n +"$((VERSIONS_TO_KEEP + 1))") - if [[ -n "$OLD_FILES" ]]; then - echo "Removing old versions..." - echo "$OLD_FILES" | xargs rm -v - fi - else - echo "Failed to download the file '$LATEST_FILE'. Exiting." - return 1 - fi + # Download the latest file (skip if already present) + if [[ -f "$LATEST_FILE" ]]; then + echo "File '$LATEST_FILE' already exists locally. No download needed." + else + echo "Downloading the latest file: $LATEST_FILE..." + if ! wget -q --show-progress "${url}${LATEST_FILE}"; then + echo "Failed to download the file '$LATEST_FILE'. Exiting." + return 1 + fi + echo "File '$LATEST_FILE' successfully downloaded to $(pwd)." + fi + + # Prune older local versions on every successful run + local OLD_FILES + OLD_FILES=$(for f in *; do [[ -f "$f" && "$f" =~ ^${pattern}$ ]] && echo "$f"; done | sort -r | tail -n +"$((VERSIONS_TO_KEEP + 1))") + if [[ -n "$OLD_FILES" ]]; then + echo "Removing old versions..." + echo "$OLD_FILES" | xargs rm -v + fiNote: also added
-f "$f"to the test so directories that happen to match the regex aren't fed torm(which would fail without-r).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/get-offline-data-file.sh` around lines 60 - 74, The early return when LATEST_FILE already exists skips the retention/prune logic, so hoist the pruning block so it executes on every successful run (both when the file is already present and after a download); specifically, move the OLD_FILES calculation and the "Removing old versions..." xargs rm step out of the wget-success branch and run it unconditionally before returning, and update the file-selection loop that sets OLD_FILES to include a file-existence guard (use -f "$f") when matching against pattern and sorting so directories aren't passed to rm; keep references to LATEST_FILE, pattern, VERSIONS_TO_KEEP, OLD_FILES, url, and wget to find and modify the relevant code.
🧹 Nitpick comments (1)
scripts/get-offline-data-file.sh (1)
70-73: Optional: harden against unusual filenames.
echo "$OLD_FILES" | xargs rm -vword-splits on whitespace. Current Kiwix/Ollama filenames are safe, but using newline-delimited input makes this future-proof:- echo "$OLD_FILES" | xargs rm -v + printf '%s\n' "$OLD_FILES" | xargs -d '\n' -r rm -v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/get-offline-data-file.sh` around lines 70 - 73, The removal step is vulnerable to word-splitting because echo "$OLD_FILES" is whitespace-split before xargs; update the cleanup to pass filenames null-delimited and use xargs -0: produce null-separated output (e.g., via printf '%s\0' for each filename stored in OLD_FILES or build OLD_FILES as a null-separated string) and call xargs -0 rm -v so rm receives correct filenames even if they contain spaces/newlines; target the OLD_FILES production and the line that pipes into xargs rm -v.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/get-offline-data-file.sh`:
- Around line 60-74: The early return when LATEST_FILE already exists skips the
retention/prune logic, so hoist the pruning block so it executes on every
successful run (both when the file is already present and after a download);
specifically, move the OLD_FILES calculation and the "Removing old versions..."
xargs rm step out of the wget-success branch and run it unconditionally before
returning, and update the file-selection loop that sets OLD_FILES to include a
file-existence guard (use -f "$f") when matching against pattern and sorting so
directories aren't passed to rm; keep references to LATEST_FILE, pattern,
VERSIONS_TO_KEEP, OLD_FILES, url, and wget to find and modify the relevant code.
---
Nitpick comments:
In `@scripts/get-offline-data-file.sh`:
- Around line 70-73: The removal step is vulnerable to word-splitting because
echo "$OLD_FILES" is whitespace-split before xargs; update the cleanup to pass
filenames null-delimited and use xargs -0: produce null-separated output (e.g.,
via printf '%s\0' for each filename stored in OLD_FILES or build OLD_FILES as a
null-separated string) and call xargs -0 rm -v so rm receives correct filenames
even if they contain spaces/newlines; target the OLD_FILES production and the
line that pipes into xargs rm -v.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2181378f-5b00-4ff7-8a52-f978ad1ec455
📒 Files selected for processing (8)
.claude/commands/update-ai-models.mdTaskfile.yamlscripts/copy-repo-template.shscripts/get-offline-data-file.shscripts/get-offline-data-ollama.shscripts/git-rebase-to-main.shscripts/infra-mcp/start-server.shscripts/jellyfin-rescan.sh
💤 Files with no reviewable changes (1)
- scripts/get-offline-data-ollama.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commits in this PR
Summary by CodeRabbit
New Features
Documentation