#5518 Add eventProvider to Heartbeat for dynamic SSE events#5572
#5518 Add eventProvider to Heartbeat for dynamic SSE events#5572bjhham merged 2 commits intoktorio:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds dynamic event generation to SSE heartbeats by introducing a new ChangesHeartbeat Event Provider
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ktor-network/ktor-network-tls/common/src/io/ktor/network/tls/extensions/SignatureAlgorithm.kt (1)
139-157: 💤 Low valueKDoc style: consider simplifying to match ktor’s minimal public API KDoc conventions.
The implementation/behavior looks consistent with the documented semantics (anonymous sign rejects via
check, unknown hash throwsTLSException, unknown signature returnsnull). However, the added KDoc uses@param,@return, and@throwstags; in ktor this project appears to follow a minimal KDoc style for documented public API extension functions (brief description + a[Report a problem]link, without requiring explicit param/return/throws tags). citeturn0search0If there’s no tooling enforcing the minimal format, this is just style/nit; otherwise, align the KDoc to the project convention to reduce review churn later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-network/ktor-network-tls/common/src/io/ktor/network/tls/extensions/SignatureAlgorithm.kt` around lines 139 - 157, Simplify the KDoc for HashAndSign.Companion.byCode to match the project's minimal public API style: replace the verbose tags (`@param`, `@return`, `@throws`) with a concise one-line description followed by the existing "[Report a problem]" link, keeping the note that the function finds or creates a HashAndSign by numeric codes and maintaining mention of anonymous-sign rejection where relevant; ensure the signature and behavior remain unchanged and that the brief KDoc references HashAndSign.Companion.byCode and SignatureAlgorithm.ANON for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-sse/common/test/io/ktor/server/sse/ServerSentEventsTest.kt`:
- Line 339: The test method name testHeartbeatEventProvider should be renamed to
a descriptive backtick-style name to follow the project's test naming
convention; locate the function declaration testHeartbeatEventProvider (and the
other similar method at the mention around line 382) in ServerSentEventsTest.kt
and change the method name to a backtick string describing what the test
verifies (for example, `heartbeat events are provided at the configured
interval`), leaving the body and call sites unchanged.
---
Nitpick comments:
In
`@ktor-network/ktor-network-tls/common/src/io/ktor/network/tls/extensions/SignatureAlgorithm.kt`:
- Around line 139-157: Simplify the KDoc for HashAndSign.Companion.byCode to
match the project's minimal public API style: replace the verbose tags (`@param`,
`@return`, `@throws`) with a concise one-line description followed by the existing
"[Report a problem]" link, keeping the note that the function finds or creates a
HashAndSign by numeric codes and maintaining mention of anonymous-sign rejection
where relevant; ensure the signature and behavior remain unchanged and that the
brief KDoc references HashAndSign.Companion.byCode and SignatureAlgorithm.ANON
for context.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58a57cef-7366-478a-8623-d3b7ac1693f9
📒 Files selected for processing (8)
ktor-network/common/src/io/ktor/network/selector/Selectable.ktktor-network/jvm/src/io/ktor/network/selector/InterestSuspensionsMap.ktktor-network/jvm/src/io/ktor/network/sockets/JavaSocketAddressUtils.ktktor-network/ktor-network-tls/common/src/io/ktor/network/tls/extensions/SignatureAlgorithm.ktktor-server/ktor-server-plugins/ktor-server-sse/api/ktor-server-sse.apiktor-server/ktor-server-plugins/ktor-server-sse/api/ktor-server-sse.klib.apiktor-server/ktor-server-plugins/ktor-server-sse/common/src/io/ktor/server/sse/ServerSSESession.ktktor-server/ktor-server-plugins/ktor-server-sse/common/test/io/ktor/server/sse/ServerSentEventsTest.kt
bjhham
left a comment
There was a problem hiding this comment.
Thanks for the contribution 👍
Nice to see some documentation filled in too.
| event = ServerSentEvent(data = "static") | ||
| eventProvider = { | ||
| ServerSentEvent(data = "dynamic") | ||
| } |
There was a problem hiding this comment.
It's a little confusing, but I can see why it's necessary to preserve backwards compatibility.
|
Oh, looks like there's a conflict with main now. Would you mind resolving this? |
9f6d1f3 to
e760508
Compare
|
Thanks for the review — fixed! |
Subsystem
Server, ktor-server-sse
Motivation
Fixes #5518.
Heartbeat.eventis captured at config time, so every tick emits the same payload — no way to embed a fresh timestamp or counter without abandoning the built-inheartbeat()helper.Solution
Add
Heartbeat.eventProvider: (suspend () -> ServerSentEvent)?. When set, the heartbeat loop invokes it on each tick; otherwise falls back toevent(existing behavior).heartbeat { period = 30.seconds eventProvider = { ServerSentEvent(data = "ts=${Clock.System.now()}") } }