feat: stream heartbeat wrapper - #25
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new ChangesHeartbeat Stream Wrapper
Sequence Diagram(s)sequenceDiagram
participant Caller
participant HeartbeatStream
participant HeartbeatCallback
participant InnerStream
Caller->>HeartbeatStream: poll_next()
HeartbeatStream->>HeartbeatStream: compute elapsed, compare heartbeat_interval
alt Interval elapsed
HeartbeatStream->>HeartbeatCallback: call(HeartbeatData { elapsed, is_timeout })
end
HeartbeatStream->>HeartbeatStream: poll timeout Sleep or check start.elapsed()
alt Timeout exceeded
HeartbeatStream->>Caller: return Err(ApiError::Api)
else Not timed out
HeartbeatStream->>InnerStream: poll_next()
InnerStream->>HeartbeatStream: return Ok(StreamEvent) / Err(ApiError)
HeartbeatStream->>Caller: forward result
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/stream/heartbeat.rs`:
- Around line 396-411: The test fires_heartbeat_on_interval is non-validating
because it creates HeartbeatStream (HeartbeatStream::new) but binds it to
`_stream` and never polls it; update the test to keep the stream (remove the
leading underscore) and actively poll it so heartbeat logic in poll_next runs
and you can assert the callback side-effect: create the callbacks
Arc<Mutex<Vec<HeartbeatData>>> via make_config, construct VecStream and
HeartbeatStream, then use futures::StreamExt::next (or pin the stream and call
poll_next once) after waiting the interval to force heartbeat execution, and
finally assert that callbacks.lock() contains at least one HeartbeatData entry
to verify the heartbeat fired.
- Around line 268-291: The current poll_next (in the stream wrapper that uses
fields last_heartbeat, start, config, on_heartbeat, and inner) only checks
Instant::elapsed() when polled, so add an internal tokio timer (e.g., a
tokio::time::Sleep stored as a field like timeout_sleep:
Option<Pin<Box<Sleep>>>) and poll it alongside the inner stream inside
poll_next; initialize or reset that Sleep to the remaining duration
(config.timeout - start.elapsed()) when the stream starts or after any
activity/heartbeat, poll the Sleep and when it completes return the same
ApiError timeout, and ensure you reset/replace the Sleep whenever
last_heartbeat/start are reset so the wrapper can wake itself even if inner
returns Poll::Pending.
🪄 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 Plus
Run ID: f2ea6dc4-7524-4219-92c4-af7d71341aa8
📒 Files selected for processing (2)
src/stream.rssrc/stream/heartbeat.rs
a4f5fd0 to
725e791
Compare
725e791 to
cf40be2
Compare
feat: stream heartbeat wrapper
No description provided.