Repository navigation
fix: replace panic!() with graceful error handling - #223
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe pull request refactors error handling across multiple modules. Function signatures in 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 88.27% 88.36% +0.08%
==========================================
Files 113 113
Lines 53215 53521 +306
==========================================
+ Hits 46977 47292 +315
+ Misses 6238 6229 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/correct.rs (1)
776-780: Consider documenting truncated record behavior.Returning
Ok(None)for records < 32 bytes treats corruption as "missing UMI." This is defensive but the doc comment (lines 760-766) doesn't mention it.📝 Suggested doc update
/// # Errors /// /// Returns an error if: /// - Records have different UMIs /// - Some records have UMIs and others don't /// - UMI tag has non-string type + /// + /// Note: Truncated records (< 32 bytes) are treated as having no UMI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/correct.rs` around lines 776 - 780, Update the doc comment above the function (the block around lines 760-766) to describe that truncated records (any entry in raw_records with length < 32) are treated as corrupted and cause the function to return Ok(None) (i.e., treated as a "missing UMI"); reference the guard that checks raw_records.iter().any(|r| r.len() < 32) and the early return Ok(None) so callers know this defensive behavior and its rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/extract.rs`:
- Around line 901-905: The loop currently treats a None from iter.next() as
end-of-reads and breaks, which silently accepts asymmetric EOFs; change the None
arm so that if any reader's iterator yields None while others still produce
reads you return an out-of-sync error instead of breaking. Concretely, in the
block handling iter.next() (the match that pushes into next_read_sets using iter
and next_read_sets), replace the None => break behavior with returning an
appropriate error (e.g., an OutOfSync/Io error or your crate's sync error) that
includes context about which reader/index went EOF, ensuring callers can detect
FASTQ truncation rather than proceeding silently.
---
Nitpick comments:
In `@src/commands/correct.rs`:
- Around line 776-780: Update the doc comment above the function (the block
around lines 760-766) to describe that truncated records (any entry in
raw_records with length < 32) are treated as corrupted and cause the function to
return Ok(None) (i.e., treated as a "missing UMI"); reference the guard that
checks raw_records.iter().any(|r| r.len() < 32) and the early return Ok(None) so
callers know this defensive behavior and its rationale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e931af75-3013-42c5-8312-c524a9bb1f1e
📒 Files selected for processing (10)
src/commands/compare/metrics.rssrc/commands/correct.rssrc/commands/dedup.rssrc/commands/extract.rssrc/lib/fastq.rssrc/lib/sort/raw_bam_reader.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/umi/parallel_assigner.rssrc/lib/unified_pipeline/deadlock.rs
Convert 10 production panic!() calls to anyhow::bail!() with proper
Result propagation for user-input validation errors in correct and
extract commands. Replace 18 test panic!() calls with unreachable!()
or error assertions for idiomatic Rust test patterns.
- correct.rs: extract_and_validate_template_umi{,_raw} now return
Result<Option<String>> instead of panicking on malformed UMI data
- fastq.rs: from_record_with_structure returns Result<Self>;
ReadSetIterator yields Result<FastqSet> items
- extract.rs: updated callers to propagate errors
- Test files: panic!() → unreachable!(), #[should_panic] → error checks
This branch was previously deployed
1 participant
Summary
panic!()calls toanyhow::bail!()with properResultpropagation for input validation errors (non-string UMI tags, mismatched UMIs, malformed FASTQ records)panic!()calls withunreachable!()(let-else branches) or.unwrap_err()assertions (#[should_panic]tests)panic!()calls that guard programmer invariants (PipelineStep::from_indexconst fn, bounded queue post-is_fullpush)Details
Production changes:
correct.rs:extract_and_validate_template_umi{,_raw}returnResult<Option<String>>instead of panicking on malformed UMI datafastq.rs:from_record_with_structurereturnsResult<Self>;ReadSetIteratoryieldsResult<FastqSet>itemsextract.rs: Updated callers to propagate errors via?and.map_err(io::Error::other)Test changes (8 files):
panic!()→unreachable!()in let-else fallback branches#[should_panic]→.unwrap_err()+assert!(err.to_string().contains(...))assert!(x.is_some())before let-else patternsTest plan
cargo ci-test— 2213 tests passcargo ci-fmt— cleancargo ci-lint— cleanpanic!()in production code except 2 intentional invariant guards