fix(sse): added JsonEvent/TextEvent and updated Join with http.ResponseWriter #60
Conversation
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
Reviewer's Guide by SourceryThis pull request introduces a Server-Sent Events (SSE) extension. It wraps the raw Sequence diagram for joining a SSE clientsequenceDiagram
participant User
participant Server
participant Client
participant Streamer
User->>Server: GET /chatroom/{id}
Server->>Server: ss.Join(ctx, id, rw)
Server->>Streamer: NewStreamer(rw)
alt rw does not implement http.Flusher
Streamer-->>Server: error
Server-->>User: error
else
Server->>Server: Check if client exists
alt Client does not exist
Server->>Client: Create new client
Server->>Server: Store client
end
Server->>Client: client.Connect(ctx, streamer)
Server-->>User: Client
end
Sequence diagram for sending a SSE eventsequenceDiagram
participant Client
participant Streamer
Client->>Client: Send(event)
alt Context is not done
Client->>Streamer: event.Write(streamer)
Streamer->>Streamer: Write event data
Streamer->>Streamer: Flush()
Client-->>Client: nil
else Context is done
Client-->>Client: ErrClientClosed
end
Updated class diagram for SSE EventclassDiagram
Event <|-- TextEvent
Event <|-- JsonEvent
class Event{
<<interface>>
Write(r io.Writer) error
}
class TextEvent{
Name string
Data string
Write(w io.Writer) error
}
class JsonEvent{
Name string
Data any
Write(w io.Writer) error
}
Updated class diagram for SSE ClientclassDiagram
Client -- Streamer : has
Client -- context.Context : has
Client : ID string
Client : s Streamer
Client : ctx context.Context
Client : cancel context.CancelFunc
Client : Connect(ctx context.Context, s Streamer)
Client : Send(event Event) error
Client : Wait()
Client : Close()
Streamer <|-- stdStreamer
class Streamer{
<<interface>>
http.ResponseWriter
http.Flusher
}
class stdStreamer{
http.ResponseWriter
http.Flusher
}
note for Client "Represents a connection to a streaming service."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment to
Clientexplaining why the context is stored on the struct. - The error wrapping in
Client.SendandServer.Broadcastseems a bit verbose; consider if a simpler approach would suffice.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
- Coverage 90.81% 90.79% -0.02%
==========================================
Files 58 61 +3
Lines 2363 2413 +50
==========================================
+ Hits 2146 2191 +45
- Misses 188 193 +5
Partials 29 29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a
Close()method to theServerto ensure all clients are properly closed when the server shuts down. - The
Connectmethod on theClientstruct could be simplified by directly callingcontext.WithCancelinstead of having a separateConnectmethod.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding an example of how to use the SSE functionality with htmx in the README.
- The error handling in
Broadcastcould be improved by returning a channel of errors instead of a slice.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return err | ||
| } | ||
|
|
||
| // JsonEvent represents an event with a name and associated data. |
There was a problem hiding this comment.
issue (complexity): Consider consolidating TextEvent and JsonEvent into a single Event struct with a flag to indicate if JSON marshaling is needed.
Consider consolidating the two event types into a unified Event struct to avoid duplicating the formatting logic. For example, you could add a flag to determine if JSON marshaling is needed:
type Event struct {
Name string
Data any
IsJSON bool
}
func (e *Event) Write(w io.Writer) error {
var dataStr string
if e.IsJSON {
buf, err := json.Marshal(e.Data)
if err != nil {
return err
}
dataStr = string(buf)
} else {
// Expecting e.Data to be a string when not using JSON.
str, ok := e.Data.(string)
if !ok {
return fmt.Errorf("data is not a string")
}
dataStr = str
}
_, err := fmt.Fprintf(w, "event: %s\ndata: %s\n\n", e.Name, dataStr)
return err
}Actionable steps:
- Replace the separate
TextEventandJsonEventtypes with the unifiedEventtype. - Use the
IsJSONflag (or similar mechanism) to decide whether to JSON marshalDataor treat it as a plain string. - Update any callers accordingly.
This approach reduces duplication while preserving functionality.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changed
Fixed
Added
Tests
Tasks to complete before merging PR:
make unit-testto check for any regressions 📋make lintto check for any issuesSummary by Sourcery
This PR enhances the SSE extension by adding a streamer implementation to handle raw HTTP responses, adding support for JSON events, and implementing a graceful shutdown mechanism. It also includes documentation updates and improved error handling.
New Features:
stdStreamerto wrap the rawhttp.ResponseWriterand ensure it implements thehttp.Flusherinterface, which is required for Server-Sent Events (SSE) to work correctly.Shutdownmethod to gracefully close all active SSE client connections.Documentation:
Tests: