chore(swift-sdk): script to get spv stortage from iOS sim#3950
Conversation
ZocoLini
requested review from
QuantumExplorer,
llbartekll and
shumkov
as code owners
📝 WalkthroughWalkthroughA new Bash script ChangesSPV Storage Extraction Utility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
✅ Review complete (commit 81ae65d) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a Bash helper to extract SwiftExampleApp SPV storage from a booted iOS Simulator, plus a matching .gitignore entry. The script is solid (set -euo pipefail, Bash 3.2 aware, interactive pickers with sane defaults) and touches no consensus/Rust code. One material reliability issue worth fixing for the script's stated debugging purpose, plus one minor robustness nit.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/get_spv_storage.sh`:
- [SUGGESTION] packages/swift-sdk/get_spv_storage.sh:248-253: Repeated runs merge stale files into the extracted SPV store
`mkdir -p "$OUT_DIR"` followed by `cp -R "$src/." "$OUT_DIR/"` overlays a fresh dump onto whatever is already in `OUT_DIR`. If the SPV store has shed/rotated/compacted segment or metadata files between runs, the older files linger in the destination and silently corrupt the snapshot. Since this script exists specifically to debug SPV storage state, a misleading hybrid dump is the worst-case failure mode. Either refuse to write into a non-empty destination, timestamp the default `OUT_DIR`, or stage into a temp dir and replace atomically.
9fc1992 to
ab4164a
Compare
ZocoLini
force-pushed
the
chore/spv-storage-script
branch
from
ab4164a to
81ae65d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/swift-sdk/get_spv_storage.sh (1)
250-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent stale snapshot overlay when
--outalready exists.Line 250 + Line 251 currently merge new SPV files into any existing directory, which can leave deleted/rotated files behind and produce a corrupted debug snapshot. Refuse pre-existing output paths (or stage to a temp dir and replace atomically).
Suggested fix
if [ -z "$OUT_DIR" ]; then stamp="$(date +%Y-%m-%dT%H-%M-%S)" OUT_DIR="spv-storage-${safe_name}-${label}-${stamp}" +elif [ -e "$OUT_DIR" ]; then + echo "Output path already exists: $OUT_DIR" >&2 + echo "Choose a different --out path or remove it first." >&2 + exit 1 fi mkdir -p "$OUT_DIR" cp -R "$src/." "$OUT_DIR/"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/get_spv_storage.sh` around lines 250 - 251, The current implementation uses cp -R to merge files into the output directory, which can leave stale files behind if OUT_DIR already exists. Fix this by either checking if OUT_DIR exists before proceeding and refusing to continue (with an appropriate error message), or by staging files to a temporary directory first and then atomically replacing the final output directory. This ensures a clean snapshot without leftover deleted or rotated files that could corrupt the debug output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/swift-sdk/get_spv_storage.sh`:
- Around line 250-251: The current implementation uses cp -R to merge files into
the output directory, which can leave stale files behind if OUT_DIR already
exists. Fix this by either checking if OUT_DIR exists before proceeding and
refusing to continue (with an appropriate error message), or by staging files to
a temporary directory first and then atomically replacing the final output
directory. This ensures a clean snapshot without leftover deleted or rotated
files that could corrupt the debug output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b86b5c3-b3d9-4ebf-a58a-59f3cbfe6e63
📒 Files selected for processing (2)
packages/swift-sdk/.gitignorepackages/swift-sdk/get_spv_storage.sh
| @@ -0,0 +1,254 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Put such things in the scripts folder inside the package or at the root level.
There was a problem hiding this comment.
the is no such folder in the swift-sdk package, and all the scripts related to the swift-sdk are directly under the package folder (build_ios.sh, verify_build.sh, get_logs.sh and setup_ios_build.sh), not inside a scripts folder, changing were the swift-sdk scripts are written is a different PR, moving all of them at once, moving one alone only causes confusion
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a developer utility shell script to extract SPV storage from the iOS Simulator and a .gitignore entry. Non-consensus-critical, self-contained. The script works as intended; only minor polish suggestions remain, the most substantive being that an explicit --out directory is silently merged into rather than checked for existing contents.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/get_spv_storage.sh`:
- [SUGGESTION] packages/swift-sdk/get_spv_storage.sh:245-251: Existing `--out` directory is silently merged into
When the caller passes `--out`, `mkdir -p` followed by `cp -R "$src/." "$OUT_DIR/"` overlays the SPV snapshot on top of whatever already lives at that path. Files that have since been rotated or deleted from the simulator store will remain in the exported directory, producing a snapshot that no longer matches the selected simulator/network — exactly the kind of corrupt artifact a debug tool must not produce. Either refuse a non-empty target or remove it atomically before copying. The default timestamped path is unaffected.
| if [ -z "$OUT_DIR" ]; then | ||
| stamp="$(date +%Y-%m-%dT%H-%M-%S)" | ||
| OUT_DIR="spv-storage-${safe_name}-${label}-${stamp}" | ||
| fi | ||
|
|
||
| mkdir -p "$OUT_DIR" | ||
| cp -R "$src/." "$OUT_DIR/" |
There was a problem hiding this comment.
🟡 Suggestion: Existing --out directory is silently merged into
When the caller passes --out, mkdir -p followed by cp -R "$src/." "$OUT_DIR/" overlays the SPV snapshot on top of whatever already lives at that path. Files that have since been rotated or deleted from the simulator store will remain in the exported directory, producing a snapshot that no longer matches the selected simulator/network — exactly the kind of corrupt artifact a debug tool must not produce. Either refuse a non-empty target or remove it atomically before copying. The default timestamped path is unaffected.
| if [ -z "$OUT_DIR" ]; then | |
| stamp="$(date +%Y-%m-%dT%H-%M-%S)" | |
| OUT_DIR="spv-storage-${safe_name}-${label}-${stamp}" | |
| fi | |
| mkdir -p "$OUT_DIR" | |
| cp -R "$src/." "$OUT_DIR/" | |
| if [ -z "$OUT_DIR" ]; then | |
| stamp="$(date +%Y-%m-%dT%H-%M-%S)" | |
| OUT_DIR="spv-storage-${safe_name}-${label}-${stamp}" | |
| fi | |
| if [ -e "$OUT_DIR" ]; then | |
| echo "Output path already exists: $OUT_DIR" >&2 | |
| echo "Choose a different --out path or remove it first." >&2 | |
| exit 1 | |
| fi | |
| mkdir -p "$OUT_DIR" | |
| cp -R "$src/." "$OUT_DIR/" |
source: ['claude', 'codex']
|
|
||
| # ---------- copy ---------- | ||
|
|
||
| safe_name="$(printf '%s' "$NAME" | tr ' /' '__')" |
There was a problem hiding this comment.
💬 Nitpick: safe_name only sanitizes spaces and slashes
Simulator names commonly include parentheses (e.g. iPhone 16 Pro (3rd generation)), which then appear unescaped in the auto-generated OUT_DIR. Most shells/filesystems tolerate this but it's awkward in tooling and history. Sanitize all non-alphanumeric characters instead.
| safe_name="$(printf '%s' "$NAME" | tr ' /' '__')" | |
| safe_name="$(printf '%s' "$NAME" | tr -c 'A-Za-z0-9._-' '_')" |
source: ['claude']
| if ! [[ "$choice" =~ ^[0-9]+$ ]] \ | ||
| || [ "$choice" -lt 1 ] || [ "$choice" -gt "${#networks[@]}" ]; then | ||
| echo "Choice out of range." >&2; exit 1 | ||
| fi |
There was a problem hiding this comment.
💬 Nitpick: Inconsistent indentation in pick_network validation
The if ! [[ ... ]] line is flush to column 1 while the continuation and then body use the function's standard 2-space indent. Purely cosmetic, but stands out next to the otherwise consistent style in the file.
| if ! [[ "$choice" =~ ^[0-9]+$ ]] \ | |
| || [ "$choice" -lt 1 ] || [ "$choice" -gt "${#networks[@]}" ]; then | |
| echo "Choice out of range." >&2; exit 1 | |
| fi | |
| if ! [[ "$choice" =~ ^[0-9]+$ ]] \ | |
| || [ "$choice" -lt 1 ] || [ "$choice" -gt "${#networks[@]}" ]; then | |
| echo "Choice out of range." >&2; exit 1 | |
| fi |
source: ['claude']
shumkov
left a comment
shumkov
left a comment
There was a problem hiding this comment.
we move to scripts in next PR
3 participants
Script to easily extract the spv storage, making issues easier to debug
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Chores