refactor: removed dead code identified in issue - #848
Conversation
- restore_session: drop unreachable hex-validity guards on PublicKey values; nostr_sdk::PublicKey always serializes to 64-char hex so the guards could never trigger - admin_take_dispute: drop is_solver == 0 recheck after find_solver_pubkey; the SQL already filters WHERE is_solver == true so the Ok arm is structurally guaranteed non-zero - db: remove rebuild_disputes_table_without_tokens and the SQLite version-fallback branch inside migrate_remove_token_columns; DROP COLUMN is supported on all deployment targets (SQLite >= 3.35 everywhere: Ubuntu 24.04 = 3.45, Debian Bookworm = 3.40, Alpine = 3.43+) - dispute: propagate setup_dispute error with map_err(MostroCantDo)? instead of silently swallowing it; the old if .is_ok() pattern skipped order.update() on failure but kept running, leaving the order flags unset while still creating the dispute row
WalkthroughThe pull request removes obsolete validation and fallback branches, propagates dispute setup failures, and replaces legacy disputes-table rebuilding with transactional removal of existing token columns. ChangesApplication and database cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/db.rs (1)
3410-3416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive-path migration test.
With
rebuild_disputes_table_preserves_rowsgone, the only remaining coverage is the no-op branch — the actual drop path (and row preservation) is untested. A test that adds the legacy columns back, inserts a row, runs the migration, and asserts both columns are gone while the row survives would keep coverage on the reachable logic this PR is consolidating onto.🧪 Sketch of the missing test
#[tokio::test] async fn migrate_remove_token_columns_drops_legacy_columns_and_preserves_rows() { let pool = migrated_pool().await; sqlx::query("ALTER TABLE disputes ADD COLUMN buyer_token INTEGER") .execute(&pool) .await .unwrap(); sqlx::query("ALTER TABLE disputes ADD COLUMN seller_token INTEGER") .execute(&pool) .await .unwrap(); // insert a dispute row here, then: migrate_remove_token_columns(&pool).await.unwrap(); assert!(!table_column_exists(&pool, "disputes", "buyer_token").await.unwrap()); assert!(!table_column_exists(&pool, "disputes", "seller_token").await.unwrap()); // assert the inserted row is still present }🤖 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 `@src/db.rs` around lines 3410 - 3416, Add a positive-path test alongside migrate_remove_token_columns_is_noop_without_token_columns that restores both legacy token columns, inserts a disputes row, runs migrate_remove_token_columns, and verifies buyer_token and seller_token are removed while the inserted row remains.
🤖 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.
Nitpick comments:
In `@src/db.rs`:
- Around line 3410-3416: Add a positive-path test alongside
migrate_remove_token_columns_is_noop_without_token_columns that restores both
legacy token columns, inserts a disputes row, runs migrate_remove_token_columns,
and verifies buyer_token and seller_token are removed while the inserted row
remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46a3b8f6-0bd3-4fdf-b711-402423c284ff
📒 Files selected for processing (4)
src/app/admin_take_dispute.rssrc/app/dispute.rssrc/app/restore_session.rssrc/db.rs
💤 Files with no reviewable changes (1)
- src/app/restore_session.rs
restore_session: drop unreachable hex-validity guards on PublicKey values; nostr_sdk::PublicKey always serializes to 64-char hex so the guards could never trigger
admin_take_dispute: drop is_solver == 0 recheck after find_solver_pubkey; the SQL already filters WHERE is_solver == true so the Ok arm is structurally guaranteed non-zero
db: remove rebuild_disputes_table_without_tokens and the SQLite version-fallback branch inside migrate_remove_token_columns; DROP COLUMN is supported on all deployment targets (SQLite >= 3.35 everywhere: Ubuntu 24.04 = 3.45, Debian Bookworm = 3.40, Alpine = 3.43+)
dispute: propagate setup_dispute error with map_err(MostroCantDo)? instead of silently swallowing it; the old if .is_ok() pattern skipped order.update() on failure but kept running, leaving the order flags unset while still creating the dispute row
Closes #818
Summary by CodeRabbit