Conversation
m3nu
left a comment
m3nu
left a comment
There was a problem hiding this comment.
🔍 Code Review
PR: #2296 - Fix better borg error
Branch: fix-better-borg-error → master
Changes: +130 / -35 lines across 15 files
📋 Summary
This PR improves error handling across all Borg job classes by adding consistent returncode checks and error messages with log links. It also adds a new error_signal mechanism to display exception dialogs for borg errors.
📏 Rules Applied
- Project config:
.claude-review.yml✗ - CLAUDE.md conventions: ✓
- Default rules: ✓
✅ What's Good
- Consistent error handling pattern applied across all borg job classes
- Proper use of
translate()with correct class contexts (mostly) - KeyboardInterrupt handling to prevent test interruptions
- Clean string concatenation fixes removing unnecessary breaks
- Good use of
config.LOG_DIR.as_uri()for log links
🔍 Review Details
3 inline comment(s) added.
| Severity | Count |
|---|---|
| 🔴 Error | 2 |
| 🟡 Warning | 1 |
| 🔵 Info | 0 |
📊 Issues Found
Please fix the duplicate pre_post_backup_cmd call and the wrong translation context before merging.
🤖 Reviewed by Claude Code
| ).format(config.LOG_DIR.as_uri()) | ||
| ) | ||
| else: | ||
| self.app.backup_log_event.emit('', {}) |
There was a problem hiding this comment.
🐛 [duplicate-call] Duplicate pre_post_backup_cmd call
pre_post_backup_cmd is already called at line 55 (before the returncode check). This line causes the post backup command to run twice when the backup succeeds.
Fix:
Remove this duplicate call:
else:
self.app.backup_log_event.emit('', {})
self.app.backup_progress_event.emit(f"[{self.params['profile_name']}] {self.tr('Backup finished.')}")
# Remove this line - already called above| def finished_event(self, result): | ||
| self.result.emit(result) | ||
| if result['returncode'] != 0: | ||
| self.app.backup_progress_event.emit( |
There was a problem hiding this comment.
🐛 [wrong-context] Wrong translation context
The translation context should be 'BorgUmountJob' not 'BorgMountJob'.
Fix:
+ translate(
'BorgUmountJob', 'Umount command has failed. See the <a href="{0}">logs</a> for details.'
).format(config.LOG_DIR.as_uri())| stdout.append(read_async(p.stdout)) | ||
| break | ||
|
|
||
| if error_msg and self.process.returncode != 0: |
There was a problem hiding this comment.
🟡 [stderr-capture] Consider capturing only relevant error lines
Currently error_msg = stderr captures the entire stderr buffer when JSON parsing fails. This could include multiple lines of output, some of which may not be relevant to the error.
Consider either:
- Capturing only the specific line that failed to parse:
error_msg = line.strip() - Or appending to a list of error messages instead of overwriting
This is a minor suggestion - the current approach will work, but the error dialog might show more information than necessary.
3 participants
Continued from #2221