chore(swift-sdk): script to get spv stortage from iOS sim#3950
Conversation
ZocoLini
requested review from
QuantumExplorer,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 53 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 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 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 c265ba1) |
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.
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
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.
shumkov
left a comment
shumkov
left a comment
There was a problem hiding this comment.
we move to scripts in next PR
ZocoLini
force-pushed
the
chore/spv-storage-script
branch
from
81ae65d to
e3604ad
Compare
ZocoLini
force-pushed
the
chore/spv-storage-script
branch
from
e3604ad to
c265ba1
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #3950 adds a developer debug script get_spv_storage.sh that extracts SwiftExampleApp's SPV storage from an iOS Simulator, plus a .gitignore entry for its outputs. No production code, FFI, consensus, or Rust paths are touched. Carried-forward findings (silent merge into existing --out, narrow safe_name sanitization) remain valid; the prior indentation issue in pick_network is now fixed. New-delta findings are limited to a minor ambiguity in --device substring matching.
💬 1 nitpick(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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