73 lines
5.2 KiB
Markdown
73 lines
5.2 KiB
Markdown
# Code Review: Feedback Loop UX
|
|
|
|
## Files Changed
|
|
|
|
| File | Change | Lines |
|
|
|------|--------|-------|
|
|
| `tidal/src/db/feedback.rs` | NEW: FeedbackAction, FeedbackState, submit_feedback, feedback_state, try_negative_preference_update, 21 unit tests | 594 |
|
|
| `tidal/src/db/mod.rs` | Added `pub mod feedback;` declaration, `skip_counter` field on TidalDb struct, `skip_counter` init in `from_config()` | +3 |
|
|
| `tidal/src/db/from_parts.rs` | Added `skip_counter: dashmap::DashMap::new()` in `from_parts()` | +1 |
|
|
| `tidal/src/entities/user_state.rs` | Added `remove_like()` and `remove_save()` methods | +12 |
|
|
| `tidal/src/lib.rs` | Added `pub use db::feedback::{FeedbackAction, FeedbackState};` | +1 |
|
|
| `tidal/tests/p1_feedback_loop.rs` | NEW: 10 integration tests | 246 |
|
|
| `tidal/src/schema/validation/policies.rs` | Added `Default` impl for `AgentPolicy` (pre-existing fix) | +9 |
|
|
| `tidal/src/session/state.rs` | Added `overrides_rejected`, `default_profile` fields (pre-existing fix) | +4 |
|
|
| `tidal/src/session/serde/start_record.rs` | Added missing field inits (pre-existing fix) | +2 |
|
|
| `tidal/src/db/sessions.rs` | Added missing field inits (pre-existing fix) | +2 |
|
|
| `tidal/src/db/session_restore.rs` | Added missing field inits (pre-existing fix) | +2 |
|
|
|
|
## Review Checklist
|
|
|
|
### Correctness
|
|
|
|
- [x] **All 11 FeedbackAction variants dispatch correctly.** Each variant in the match arm performs the exact side effects specified in the design dispatch table.
|
|
- [x] **Signal-writing actions use the correct signal type names.** Like -> "like", Hide -> "hide", Dislike -> "dislike", MuteCreator -> "block", NotInterested -> "skip".
|
|
- [x] **Undo actions reverse the correct state.** Unlike removes from liked bitmap. Unhide removes from hard-neg AND hidden-items. UndoDislike removes from hard-neg. UnmuteCreator removes from blocked-creators. Unsave removes from saved bitmap.
|
|
- [x] **Unhide writes a durable signal** (weight=-1.0) for audit trail and replication, as specified.
|
|
- [x] **Skip escalation threshold is correct.** `SKIP_ESCALATION_THRESHOLD = 3`. Uses `saturating_add` to prevent u8 overflow.
|
|
- [x] **Negative preference update uses correct damping.** Dislike: 0.3x, NotInterested: 0.1x. Base damping 0.1 matches `PreferenceVectors::DAMPING`.
|
|
- [x] **feedback_state distinguishes Hide from Dislike.** Hide = in hidden_items bitmap. Dislike = in hard_neg but NOT in hidden_items.
|
|
- [x] **`require_writeable` guard** prevents writes on follower nodes.
|
|
|
|
### API Design
|
|
|
|
- [x] **FeedbackAction derives Copy.** All variants are data-free or contain `u64`, making the enum trivially copyable. This avoids unnecessary cloning on the call site.
|
|
- [x] **FeedbackState derives Default.** All-false default is the correct semantic for "no feedback recorded."
|
|
- [x] **Public API surface is minimal.** Only `submit_feedback`, `feedback_state`, `FeedbackAction`, and `FeedbackState` are public. `try_negative_preference_update` is private.
|
|
- [x] **Method signatures match the spec.** `submit_feedback(user_id: u64, entity_id: EntityId, action: FeedbackAction, timestamp: Timestamp) -> Result<()>`.
|
|
|
|
### Safety
|
|
|
|
- [x] **`#[allow(clippy::cast_possible_truncation)]`** on `entity_id.as_u64() as u32`. This is an existing pattern throughout the codebase (EntityId is u64 but bitmap indexes use u32).
|
|
- [x] **`#[allow(clippy::struct_excessive_bools)]`** on FeedbackState. Justified: 5 independent boolean UI toggle states are the natural representation.
|
|
- [x] **No panics in production paths.** All fallible operations return `Result` or use `let...else` for early returns. `try_negative_preference_update` silently returns on missing storage/embedding (graceful degradation).
|
|
- [x] **DashMap concurrency is safe.** All accesses use the DashMap API correctly (entry/get_mut/contains patterns). No manual locking.
|
|
|
|
### Performance
|
|
|
|
- [x] **No heap allocation on undo paths.** Unlike, UndoDislike, UnmuteCreator, Unsave only perform DashMap removals.
|
|
- [x] **Skip counter uses u8** (not u64) to minimize memory per entry.
|
|
- [x] **feedback_state is read-only** with no I/O (4 DashMap reads, ~400ns total).
|
|
- [x] **No blocking I/O in any path** except the WAL write in signal-writing actions (existing cost).
|
|
|
|
### Test Coverage
|
|
|
|
- [x] **21 unit tests** cover all action variants, undo paths, skip escalation, edge cases (undo on untouched items, coexisting states).
|
|
- [x] **10 integration tests** verify roundtrip behavior, multi-user independence, preference vector updates, combined actions.
|
|
- [x] **Full lib suite passes** (1345 tests, 0 failures).
|
|
|
|
### Style
|
|
|
|
- [x] **Follows existing codebase patterns.** Module structure matches `db/signals.rs`, `db/sessions.rs`. Naming matches existing conventions.
|
|
- [x] **Doc comments on all public items.** `submit_feedback`, `feedback_state`, `FeedbackAction`, `FeedbackState` all have rustdoc with examples and error conditions.
|
|
- [x] **No dead code.** All types and methods are exercised by tests.
|
|
- [x] **cargo fmt clean.** No formatting issues.
|
|
- [x] **cargo clippy clean** for all feedback module files (0 warnings).
|
|
|
|
## Issues Found
|
|
|
|
None. The implementation matches the spec and design exactly. All acceptance criteria are met.
|
|
|
|
## Verdict
|
|
|
|
**APPROVED.** Clean implementation with comprehensive test coverage. Ready for QA verification. |