Uh oh!
There was an error while loading. Please reload this page.
refactor: show beta notices whenever stderr is a terminal - #336
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
📝 WalkthroughWalkthroughStorage beta notifications now use a ChangesBeta notice flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CatalogCommand
participant BetaFeature
participant output_notice_once
participant config_claim_notice
CatalogCommand->>BetaFeature: notify_once()
BetaFeature->>output_notice_once: pass feature id and notice
output_notice_once->>config_claim_notice: claim_notice(id)
config_claim_notice-->>output_notice_once: claim result
output_notice_once-->>CatalogCommand: emit notice when eligible
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
| match config::claim_notice(id) { | ||
| Ok(true) => notice_to_stderr(label, msg), | ||
| Ok(false) => {} | ||
| Err(err) => debug!("Failed to persist CLI notice {}: {}", id, err), |
There was a problem hiding this comment.
not realllly sure we need this, but wanted to keep the diff here clean as a move
There was a problem hiding this comment.
This writes locally (similar to session persistence). Keep it
| match config::claim_notice(id) { | ||
| Ok(true) => notice_to_stderr(label, msg), | ||
| Ok(false) => {} | ||
| Err(err) => debug!("Failed to persist CLI notice {}: {}", id, err), |
There was a problem hiding this comment.
This writes locally (similar to session persistence). Keep it
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/tower-cmd/src/beta.rs (1)
34-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain coverage for the new notice-gating path.
The refactor removes the existing terminal/claiming test while introducing
output::notice_onceas the central behavior. Add tests for non-terminal stderr, first claim, already-claimed notices, and persistence failures—ideally inoutput.rs—so regressions in the dispatch contract are caught.🤖 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 `@crates/tower-cmd/src/beta.rs` around lines 34 - 67, Add coverage around output::notice_once for the notice-gating dispatch contract: test non-terminal stderr behavior, successful first claiming, suppression of already-claimed notices, and persistence failures. Place these tests in output.rs where possible, and retain the existing BetaFeature notice formatting tests in beta.rs.
🤖 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 `@crates/tower-cmd/src/beta.rs`:
- Around line 34-67: Add coverage around output::notice_once for the
notice-gating dispatch contract: test non-terminal stderr behavior, successful
first claiming, suppression of already-claimed notices, and persistence
failures. Place these tests in output.rs where possible, and retain the existing
BetaFeature notice formatting tests in beta.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a5701b3-e208-4e79-9121-29a82b59477f
📒 Files selected for processing (3)
crates/tower-cmd/src/beta.rscrates/tower-cmd/src/catalogs.rscrates/tower-cmd/src/output.rs
In keeping with #322 this refactors the beta code to not use stdout/stderr stuff directly from non-
output.rsrelated codeSummary by CodeRabbit