chore: use channels for browser src updates #190
Conversation
also remove tracking state model
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to several files, primarily focusing on enhancing the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
pkg/server/server.go
Outdated
|
|
||
| for match := range b.matchChan { | ||
| log.Print("[w] straem match browser src", match) |
| nosqlDb *nosql.Storage, | ||
| txtDb *txt.Storage, | ||
| cfg *config.Config, | ||
| matchChans ...chan model.Match, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
pkg/tracker/t8/track.go (3)
Line range hint
81-84: Improve error handling in Poll methodThe error from
GetLastReplayis not being communicated back to the caller. Consider returning the error or logging it before canceling.Here's a suggested improvement:
func (t *T8Tracker) Poll(ctx context.Context, cancel context.CancelFunc, session *model.Session, onNewMatch func(model.Match)) { lastReplay, err := t.wavuClient.GetLastReplay(session.UserId) if err != nil { + fmt.Printf("Error getting last replay: %v\n", err) cancel() + return }
Line range hint
83-84: Clarify the use of cancel functionThe
cancelfunction is called on error, but its purpose is not clear without more context. Consider adding a comment explaining why cancellation is necessary here.Add a comment explaining the cancellation:
if err != nil { + // Cancel the context to stop any ongoing operations cancel() return }
Line range hint
80-93: LGTM with suggestions for improvementThe overall changes to the
Pollmethod look good. The transition from a channel-based approach to a callback function can provide more flexibility. The logic for checking and processing new matches remains sound.Some suggestions for further improvement:
- Consider adding logging or telemetry to track the frequency and success of polling operations.
- The
prevMatchretrieval assumes the first match in the session is the most recent. Verify if this assumption always holds true, or if additional sorting might be necessary.Here's a suggested improvement for
prevMatchretrieval:var prevMatch *model.Match if len(session.Matches) > 0 { - prevMatch = session.Matches[0] + // Ensure we're getting the most recent match + prevMatch = &session.Matches[len(session.Matches)-1] }pkg/server/server.go (1)
66-66: Typo in log message: "straem" should be "stream"The log message at line 66 contains a typo. "straem" should be corrected to "stream" for clarity.
Apply this diff to fix the typo:
-log.Print("[w] straem match browser src", match) +log.Print("[w] stream match browser src", match)cmd/tracking.go (1)
126-130: Consistent error logging and error handlingThe error logs at lines 126 and 130 print the error messages but do not perform any additional error handling or retries. Consistent error handling strategies enhance maintainability and user experience.
Consider implementing error handling mechanisms such as retries, user notifications, or detailed logging. For example:
if err := ch.sqlDb.UpdateSession(ctx, session); err != nil { log.Println("Failed to update session:", err) // Handle the error appropriately } if err := ch.sqlDb.SaveMatch(ctx, match); err != nil { log.Println("Failed to save match to database:", err) // Handle the error appropriately }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- cmd/tracking.go (5 hunks)
- main.go (3 hunks)
- pkg/model/tracking_state.go (0 hunks)
- pkg/server/server.go (2 hunks)
- pkg/tracker/sf6/track.go (2 hunks)
- pkg/tracker/t8/track.go (2 hunks)
- pkg/tracker/tracker.go (1 hunks)
💤 Files with no reviewable changes (1)
- pkg/model/tracking_state.go
🧰 Additional context used
🪛 GitHub Check: go-lint
cmd/tracking.go
[failure] 76-76:
ineffectual assignment to matchChan (ineffassign)
🔇 Additional comments (8)
pkg/tracker/tracker.go (1)
11-11: Approve the change to callback function, but verify impact on existing implementations.The modification of the
Pollmethod signature from using a channel to a callback function is a good improvement. It aligns with the PR objective of updating how browser source updates are handled and can potentially improve performance and flexibility.However, this change will require updates to all existing implementations of the
GameTrackerinterface.To ensure all implementations are updated correctly, please run the following verification script:
This script will help identify all implementations of the
GameTrackerinterface and show theirPollmethod signatures. Please review the output to ensure all implementations have been updated to match the new signature.pkg/tracker/t8/track.go (1)
80-80:⚠️ Potential issueBreaking change in method signature
The
Pollmethod signature has changed from using a channel to a callback function. This is a breaking change that will require updates in all calling code.To ensure all callers have been updated, run the following script:
If any results are found, they need to be updated to use the new callback-based approach.
✅ Verification successful
Polling Method Signature Update Verified
All usages of the old
Pollmethod signature have been successfully updated to use the new callback-based approach. No remaining references to the channel-based signature (chan model.Match) were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining uses of the old Poll method signature rg --type go 'Poll\s*\([^)]*chan\s+model\.Match[^)]*\)' -g '!pkg/tracker/t8/track.go'Length of output: 87
Script:
#!/bin/bash # Search for all uses of the Poll method to ensure no remaining old signatures rg --type go 'Poll\s*\(' -g '!pkg/tracker/t8/track.go'Length of output: 619
pkg/tracker/sf6/track.go (2)
Line range hint
1-182: Overall assessment of changesThe primary change in this file is the modification of the Poll method to use a callback function instead of a channel. This change is significant but localized, with the rest of the file remaining largely unchanged. The preservation of other methods and logic suggests a focused, intentional change rather than a broad refactoring.
The changes appear to be well-implemented and maintain the overall structure and functionality of the SF6Tracker. However, ensure that all dependent code has been updated to accommodate this API change, as suggested in the previous comment.
Line range hint
73-82: Significant API change in Poll methodThe Poll method signature has been updated to use a callback function instead of a channel for new match notifications. This change has several implications:
- It simplifies the method's usage by directly invoking the callback instead of managing a channel.
- It may impact the concurrency model of the application, potentially affecting how multiple consumers handle new match data.
- All code that previously called this method will need to be updated to provide a callback function instead of a channel.
The core logic of the method remains intact, which is good for maintaining existing behavior.
Consider the following:
- Ensure that all callers of this method have been updated to use the new signature.
- Verify that this change aligns with the overall concurrency strategy of the application.
- Update any documentation or comments related to this method to reflect the new usage pattern.
To ensure all callers have been updated, run the following script:
If this script returns any results, those locations need to be updated to use the new callback-based approach.
main.go (1)
205-205: Ensure all event listeners handle variadic arguments inEventsEmit.By passing
optionalData...toruntime.EventsEmit, verify that all frontend event listeners are expecting and can handle additional data arguments to prevent unexpected behavior or runtime errors.cmd/tracking.go (3)
42-49: Initialization ofTrackingHandlerwith multiple match channelsThe
NewTrackingHandlerfunction now accepts a variadic parametermatchChans ...chan model.Match, allowing theTrackingHandlerto manage multiple match notification channels.This change enhances the flexibility of the
TrackingHandler, enabling it to broadcast match notifications to multiple consumers.
95-98:⚠️ Potential issuePotential deadlock when broadcasting matches to multiple channels
In the
onNewMatchfunction, sending thematchto all channels inch.matchChanswithout ensuring that each channel has an active receiver may cause a deadlock. Since unbuffered channels block the sender until the receiver is ready, if any channel is not being consumed, the send operation will hang.Consider the following solutions:
- Ensure all channels have active consumers: Verify that every channel in
ch.matchChansis being read elsewhere in the codebase.- Use buffered channels: Implement buffered channels to prevent the send operation from blocking if the receiver isn't ready immediately.
- Selective broadcasting: If only certain channels need to receive the match notification, send to those specific channels instead of all.
To verify, you can check where each channel in
ch.matchChansis being consumed.
70-71:⚠️ Potential issueInconsistent consumption of match channels
A new
matchChanis created and appended toch.matchChans, but only thismatchChanis being consumed in the loop starting at line 115:for match := range matchChan { // ... }If there are other channels in
ch.matchChans, they are not being read from, which could lead to deadlocks when sending matches to them.Ensure that all channels in
ch.matchChansare being properly consumed. If onlymatchChanis necessary, consider revising the implementation to manage a single channel or remove unused channels to prevent potential deadlocks.Also applies to: 115-115
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
pkg/server/server.go (2)
Line range hint
80-94: Fix path parameter extraction inhandleThememethodThe
handleThememethod has been successfully converted to work with theBrowserSourceServerstruct. However, there's an issue with the path parameter extraction.The
http.Requesttype does not have aPathValuemethod. To fix this, you need to use thegorilla/muxpackage for extracting path variables. Here's how you can modify the code:+import "github.com/gorilla/mux" func (b *BrowserSourceServer) handleTheme(w http.ResponseWriter, req *http.Request) { - fileName := req.PathValue("theme") + vars := mux.Vars(req) + fileName := vars["theme"] css, err := staticFs.ReadFile(fmt.Sprintf("static/themes/%s", fileName)) if err != nil { w.WriteHeader(http.StatusNotFound) } else { w.Header().Set("Content-Type", "text/css") w.WriteHeader(http.StatusOK) _, err := w.Write(css) if err != nil { log.Println("failed to write browser source css") } } }This change ensures that the path parameter is correctly extracted using the
gorilla/muxpackage.
Line range hint
1-127: Overall improvement in code structure with some remaining issuesThe restructuring of this file with the introduction of
BrowserSourceServeris a significant improvement. It encapsulates related functionality and state, making the code more maintainable and easier to reason about. The conversion of functions to methods is consistent and appropriate.However, there are still some issues that need to be addressed:
- The routing mechanism needs to be updated to use a third-party router like
gorilla/muxfor proper method-based and parameterized routing.- Error handling in the
GetInternalThemesfunction needs improvement.- The
handleStreammethod needs a small adjustment in the order of operations to ensureb.lastMatchis always valid.Once these issues are resolved, the code will be in excellent shape. Great work on the overall restructuring!
Consider adding some error logging throughout the code, especially in the
Startmethod, to make debugging easier in production environments. Also, you might want to add some basic metrics collection (e.g., number of connections, number of matches sent) to monitor the performance of the browser source server.cmd/tracking.go (1)
96-102: Clarify exclusion of the last channel when sending matchesIn lines 100-102, the code sends the match to all channels except the last one:
for _, mc := range ch.matchChans[:len(ch.matchChans)-1] { mc <- match }This relies on the positional order of channels in the slice, which can be error-prone. For better readability and to prevent potential bugs, consider explicitly specifying which channels should receive the match.
You might introduce separate slices or clearly document the purpose of each channel to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- cmd/init_test.go (1 hunks)
- cmd/tracking.go (4 hunks)
- main.go (3 hunks)
- pkg/server/server.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (8)
cmd/init_test.go (1)
31-31: Verify the updatedNewTrackingHandlerfunction signatureThe
NewTrackingHandlerfunction now accepts an additional parameter, which is set tonilin this test initialization. This change in the function signature might have broader implications across the codebase.To ensure this change is consistently applied, let's check for other usages of
NewTrackingHandler:Additionally, it would be helpful to understand the purpose of this new parameter:
- What does this new parameter represent?
- Are there cases where it should be non-nil?
- Has the
NewTrackingHandlerfunction in thecmdpackage been updated to handle this new parameter correctly?Please provide more context about this change to ensure it's properly implemented and tested.
pkg/server/server.go (2)
20-29: LGTM: Well-structured encapsulation of browser source server stateThe introduction of the
BrowserSourceServerstruct and its constructor functionNewBrowserSourceServeris a good design choice. It encapsulates the necessary state (matchChanandlastMatch) and provides a clean way to initialize the server. This structure will make the code more maintainable and easier to reason about.
96-108: LGTM: Proper conversion ofhandleRootto a methodThe
handleRootfunction has been successfully converted to a method ofBrowserSourceServer. The change in setting the content type header from backticks to double quotes is a minor but welcome improvement in code style.main.go (5)
28-28: LGTM: New imports added correctly.The new imports for
modelandserverpackages are correctly added and align with the new functionality introduced in the file.
154-155: LGTM: Buffered channel implemented as suggested.The
browserSrcMatchChanis correctly implemented as a buffered channel with a capacity of 1. This addresses the previous review comment about potential blocking issues with unbuffered channels. Good job on implementing this improvement.
157-157: LGTM: TrackingHandler updated with new channel parameter.The
NewTrackingHandlerfunction call has been correctly updated to include thebrowserSrcMatchChanas an additional parameter. This change is consistent with the new functionality for handling browser source matches.
160-161: LGTM: BrowserSourceServer correctly initialized.The
browserSrcServeris properly initialized using theNewBrowserSourceServerfunction, with thebrowserSrcMatchChancorrectly passed as an argument. This change aligns with the introduction of the newBrowserSourceServertype mentioned in the summary.
210-210:⚠️ Potential issueConsider implementing error handling and graceful shutdown for browserSrcServer.
While starting
browserSrcServerin a goroutine is a good approach for non-blocking execution, the current implementation doesn't handle potential errors that might occur during the server start process. Additionally, there's no mechanism for graceful shutdown of the server when the application terminates.To improve robustness and resource management:
- Implement error handling for
browserSrcServer.Start.- Ensure graceful shutdown of
browserSrcServerwhen the application terminates.Here's a suggested implementation:
go func() { if err := browserSrcServer.Start(ctx, &cfg); err != nil { log.Printf("browserSrcServer encountered an error: %v", err) // Implement error handling logic here } }()Also, consider adding shutdown logic for
browserSrcServerin theOnShutdownfunction:OnShutdown: func(_ context.Context) { appBrowser.Page.Browser().Close() browserSrcServer.Stop() // Assuming Stop method exists },These changes will enhance the application's stability and resource management.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
BrowserSourceServertype for improved organization.Chores
trackingHandlerto accommodate new parameters.