Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an EventClient that connects to an /events WebSocket endpoint, parses incoming JSON into event.Event values, delivers them on a buffered channel, and automatically reconnects with exponential backoff. Provides options for event-type filtering and optional StatusTracker integration. Introduces a concurrency-safe StatusTracker with observer callbacks and stringified status constants. Includes comprehensive tests for event delivery, reconnection, filtering, lifecycle behavior, and status tracking. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tray/events_test.go">
<violation number="1" location="tray/events_test.go:173">
P2: `receivedPath` is written from the HTTP handler goroutine and read later in the test without synchronization, which will trigger a data race under the race detector. Consider protecting it with a mutex or using a channel to pass the value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tray/events_test.go
Outdated
| var receivedPath string | ||
|
|
||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| receivedPath = r.URL.RawQuery |
There was a problem hiding this comment.
P2: receivedPath is written from the HTTP handler goroutine and read later in the test without synchronization, which will trigger a data race under the race detector. Consider protecting it with a mutex or using a channel to pass the value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/events_test.go, line 173:
<comment>`receivedPath` is written from the HTTP handler goroutine and read later in the test without synchronization, which will trigger a data race under the race detector. Consider protecting it with a mutex or using a channel to pass the value.</comment>
<file context>
@@ -0,0 +1,288 @@
+ var receivedPath string
+
+ handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ receivedPath = r.URL.RawQuery
+ conn, err := testUpgrader.Upgrade(w, r, nil)
+ if err != nil {
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tray/events.go (2)
59-62: Defensively copytypesto avoid external mutation side effects.The option currently stores the caller’s slice reference directly. Copying avoids accidental post-construction mutation and race-prone behavior.
💡 Suggested refactor
func WithEventTypes(types []string) EventClientOption { + typeFilter := append([]string(nil), types...) return func(c *EventClient) { - c.typeFilter = types + c.typeFilter = typeFilter } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tray/events.go` around lines 59 - 62, WithEventTypes currently stores the caller's slice reference into EventClient.typeFilter, which permits external mutation; modify WithEventTypes to defensively copy the input slice before assigning (e.g., allocate a new slice with make/append or copy) so EventClient.typeFilter holds an independent slice and cannot be mutated by callers after construction.
248-253: Add observability for dropped events under backpressure.Dropping on full channel is valid, but currently silent. A counter or debug log here would make pressure and loss visible in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tray/events.go` around lines 248 - 253, The non-blocking send to c.events silently drops events when the channel is full; update the default branch to record this loss by incrementing an observability metric and emitting a debug/info log. Specifically, in the select default path where the code currently comments "// channel full, drop event", call your metrics counter (e.g., EventsDropped.Inc or a Prometheus counter) and log the dropped event with the component logger (e.g., c.logger.Debugf or similar) including identifying details from evt so backpressure and loss are visible in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tray/events_test.go`:
- Around line 170-174: The test currently writes to receivedPath in the HTTP
handler goroutine and reads it from the test goroutine, causing race-detector
failures; fix by synchronizing access — e.g., have the handler send the path
string over a channel (or store it in an atomic.Value or protect with a
sync.Mutex) instead of writing a shared variable, and have the test goroutine
receive/read from that channel/atomic/mutex-protected variable when asserting;
update the handler and the test code that reads receivedPath (references:
receivedPath variable, handler http.HandlerFunc, and testUpgrader usage) to use
this synchronization.
In `@tray/events.go`:
- Around line 67-70: WithEventStatusTracker should guard against a nil
StatusTracker: change the returned option to check if t == nil and, in that
case, return a no-op option (e.g., func(c *EventClient) {}) instead of setting
c.status to nil; otherwise assign c.status = t. This prevents
Start()/connectLoop() from panicking when they call c.status.Set(...) while
preserving existing behavior when a valid StatusTracker is provided.
In `@tray/status.go`:
- Around line 72-84: The observer callback in StatusTracker.Set is invoked
inline (obs(s)) which can block callers; change it to dispatch the callback
asynchronously by launching it in a new goroutine (e.g., go func() { ... }()),
capturing the copied obs and s variables already taken from StatusTracker, and
protect the goroutine by recovering from panics so a faulty observer cannot
crash the process; update the Set method (referenced as StatusTracker.Set and
the local obs variable) to perform non-blocking, panic-safe invocation of the
observer.
---
Nitpick comments:
In `@tray/events.go`:
- Around line 59-62: WithEventTypes currently stores the caller's slice
reference into EventClient.typeFilter, which permits external mutation; modify
WithEventTypes to defensively copy the input slice before assigning (e.g.,
allocate a new slice with make/append or copy) so EventClient.typeFilter holds
an independent slice and cannot be mutated by callers after construction.
- Around line 248-253: The non-blocking send to c.events silently drops events
when the channel is full; update the default branch to record this loss by
incrementing an observability metric and emitting a debug/info log.
Specifically, in the select default path where the code currently comments "//
channel full, drop event", call your metrics counter (e.g., EventsDropped.Inc or
a Prometheus counter) and log the dropped event with the component logger (e.g.,
c.logger.Debugf or similar) including identifying details from evt so
backpressure and loss are visible in production.
tray/events_test.go
Outdated
Comment on lines
+170
to
+174
| var receivedPath string | ||
|
|
||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| receivedPath = r.URL.RawQuery | ||
| conn, err := testUpgrader.Upgrade(w, r, nil) |
There was a problem hiding this comment.
Synchronize receivedPath access to avoid race-detector failures.
receivedPath is written from the server handler goroutine and read from the test goroutine without synchronization.
💡 Suggested fix
func TestEventClient_TypeFilter(t *testing.T) {
// Verify that the type filter is sent as a query parameter
- var receivedPath string
+ var (
+ receivedPath string
+ pathMu sync.Mutex
+ )
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- receivedPath = r.URL.RawQuery
+ pathMu.Lock()
+ receivedPath = r.URL.RawQuery
+ pathMu.Unlock()
conn, err := testUpgrader.Upgrade(w, r, nil)
if err != nil {
return
@@
waitForStatus(t, client.status, StatusConnected, 2*time.Second)
- assert.Contains(t, receivedPath, "types=")
- assert.Contains(t, receivedPath, "input.block")
- assert.Contains(t, receivedPath, "input.transaction")
+ pathMu.Lock()
+ gotPath := receivedPath
+ pathMu.Unlock()
+ assert.Contains(t, gotPath, "types=")
+ assert.Contains(t, gotPath, "input.block")
+ assert.Contains(t, gotPath, "input.transaction")
}Also applies to: 201-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tray/events_test.go` around lines 170 - 174, The test currently writes to
receivedPath in the HTTP handler goroutine and reads it from the test goroutine,
causing race-detector failures; fix by synchronizing access — e.g., have the
handler send the path string over a channel (or store it in an atomic.Value or
protect with a sync.Mutex) instead of writing a shared variable, and have the
test goroutine receive/read from that channel/atomic/mutex-protected variable
when asserting; update the handler and the test code that reads receivedPath
(references: receivedPath variable, handler http.HandlerFunc, and testUpgrader
usage) to use this synchronization.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
feature/tray-event-client
branch
from
7f2f1ef to
e439225
Compare
Member
Author
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
agaffney
approved these changes
Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Adds a tray EventClient that streams events from adder’s /events WebSocket, auto-reconnects, and exposes connection status. Includes a thread-safe StatusTracker and optional event type filtering.
Written for commit e439225. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests