-
Notifications
You must be signed in to change notification settings - Fork 0
Live score #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Live score #109
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,70 @@ | ||||||||||||||||||||
| package com.cornellappdev.score.model | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import android.util.Log | ||||||||||||||||||||
| import com.cornellappdev.score.BuildConfig | ||||||||||||||||||||
| import io.socket.client.IO | ||||||||||||||||||||
| import io.socket.client.Socket | ||||||||||||||||||||
| import kotlinx.coroutines.CoroutineScope | ||||||||||||||||||||
| import kotlinx.coroutines.flow.MutableSharedFlow | ||||||||||||||||||||
| import kotlinx.coroutines.flow.SharedFlow | ||||||||||||||||||||
| import kotlinx.coroutines.flow.asSharedFlow | ||||||||||||||||||||
| import kotlinx.coroutines.launch | ||||||||||||||||||||
| import kotlinx.serialization.json.Json | ||||||||||||||||||||
| import org.json.JSONObject | ||||||||||||||||||||
| import java.util.Collections | ||||||||||||||||||||
| import javax.inject.Inject | ||||||||||||||||||||
| import javax.inject.Singleton | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private const val TAG = "SocketManager" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Singleton | ||||||||||||||||||||
| class SocketManager @Inject constructor(private val appScope: CoroutineScope) { | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private val _gameUpdateFlow = | ||||||||||||||||||||
| MutableSharedFlow<SocketGameUpdateEnvelope>(extraBufferCapacity = 16) | ||||||||||||||||||||
| val gameUpdateFlow: SharedFlow<SocketGameUpdateEnvelope> = _gameUpdateFlow.asSharedFlow() | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private val activeSubscriptions: MutableSet<String> = | ||||||||||||||||||||
| Collections.synchronizedSet(mutableSetOf()) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private val json = Json { ignoreUnknownKeys = true; isLenient = true } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private val socket: Socket by lazy { | ||||||||||||||||||||
| val opts = IO.Options.builder() | ||||||||||||||||||||
| .setTransports(arrayOf("websocket")) | ||||||||||||||||||||
| .build() | ||||||||||||||||||||
| IO.socket(BuildConfig.SOCKET_URL, opts).also { s -> | ||||||||||||||||||||
| // "on" is a listener | ||||||||||||||||||||
| s.on(Socket.EVENT_CONNECT) { | ||||||||||||||||||||
| Log.d(TAG, "Connected") | ||||||||||||||||||||
| activeSubscriptions.forEach { id -> | ||||||||||||||||||||
| s.emit("subscribe", JSONObject().put("gameId", id)) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+40
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
file="app/src/main/java/com/cornellappdev/score/model/SocketManager.kt"
ast-grep outline "$file" --items all
sed -n '27,43p' "$file"
# After the fix, expect the reconnect handler to snapshot under the same monitor.
rg -n -C 3 'synchronized\(activeSubscriptions\)|activeSubscriptions\.toList\(\)' "$file"Repository: cuappdev/score-android Length of output: 1571 🏁 Script executed: #!/bin/bash
set -euo pipefail
file="app/src/main/java/com/cornellappdev/score/model/SocketManager.kt"
cat -n "$file"
printf '\n-- usages --\n'
rg -n -C 4 'SocketManager|subscribe\(|unsubscribe\(|EVENT_CONNECT|activeSubscriptions' app/src/main app/src/test 2>/dev/null || trueRepository: cuappdev/score-android Length of output: 12501 🤖 get_repo_knowledge executed:
Length of output: 596 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,140p' app/src/main/java/com/cornellappdev/score/model/SocketManager.kt
printf '\n-- direct references --\n'
rg -n -C 3 'SocketManager|activeSubscriptions|EVENT_CONNECT|\.subscribe\(|\.unsubscribe\(' app/src/main/java app/src/test 2>/dev/null || trueRepository: cuappdev/score-android Length of output: 10439 🌐 Web query:
💡 Result: In Java 8, while Collections.synchronizedSet returns a thread-safe wrapper, it does not automatically synchronize iteration [1][2]. The documentation explicitly states that it is imperative for the user to manually synchronize on the returned set when iterating over it to ensure thread safety [1][2]. Regarding the forEach method, the implementation in JDK 8 for synchronized collections does include internal synchronization. In the SynchronizedCollection class (the internal implementation used by Collections.synchronizedSet), the forEach method is overridden to acquire the synchronization mutex before calling the forEach method of the underlying collection [3][4]: Citations:
🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' "-- tool availability --"
command -v kotlinc || true
command -v javap || true
printf '%s\n' "-- relevant source and declarations --"
sed -n '1,120p' app/src/main/java/com/cornellappdev/score/model/SocketManager.kt
rg -n -C 4 'fun (subscribe|unsubscribe)|activeSubscriptions|EVENT_CONNECT' app/src/main/java/com/cornellappdev/score/modelRepository: cuappdev/score-android Length of output: 7411 🌐 Web query:
💡 Result: When using Kotlin's Citations:
Synchronize the reconnect subscription snapshot. This Kotlin Proposed fix- activeSubscriptions.forEach { id ->
+ val subscriptions = synchronized(activeSubscriptions) {
+ activeSubscriptions.toList()
+ }
+ subscriptions.forEach { id ->
s.emit("subscribe", JSONObject().put("gameId", id))
}Add a regression test that subscribes or unsubscribes during reconnect. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| } | ||||||||||||||||||||
| s.on(Socket.EVENT_DISCONNECT) { args -> | ||||||||||||||||||||
| Log.d( | ||||||||||||||||||||
| TAG, | ||||||||||||||||||||
| "Disconnected: ${args.firstOrNull()}" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| s.on(Socket.EVENT_CONNECT_ERROR) { args -> Log.e(TAG, "Error: ${args.firstOrNull()}") } | ||||||||||||||||||||
| s.on("game_update") { args -> | ||||||||||||||||||||
| val raw = args.firstOrNull() as? JSONObject ?: return@on | ||||||||||||||||||||
| runCatching { json.decodeFromString<SocketGameUpdateEnvelope>(raw.toString()) } | ||||||||||||||||||||
| .onSuccess { appScope.launch { _gameUpdateFlow.emit(it) } } | ||||||||||||||||||||
| .onFailure { Log.e(TAG, "Parse error: $it") } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| s.connect() | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| fun subscribe(gameId: String) { | ||||||||||||||||||||
| activeSubscriptions.add(gameId) | ||||||||||||||||||||
| socket.emit("subscribe", JSONObject().put("gameId", gameId)) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| fun unsubscribe(gameId: String) { | ||||||||||||||||||||
| activeSubscriptions.remove(gameId) | ||||||||||||||||||||
| socket.emit("unsubscribe", JSONObject().put("gameId", gameId)) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package com.cornellappdev.score.model | ||
|
|
||
| import kotlinx.serialization.Serializable | ||
|
|
||
| @Serializable | ||
| data class SocketGameUpdateEnvelope( | ||
| val type: String, | ||
| val gameId: String, | ||
| val timestamp: String, | ||
| val data: SocketGameUpdateData | ||
| ) | ||
|
|
||
| @Serializable | ||
| data class SocketGameUpdateData( | ||
| val homeScore: Int? = null, | ||
| val oppScore: Int? = null, | ||
| val scoreBreakdown: List<List<String?>?>? = null, | ||
| val boxScore: List<SocketBoxScoreEntry?>? = null | ||
| ) | ||
|
|
||
| @Serializable | ||
| data class SocketBoxScoreEntry( | ||
| val team: String? = null, | ||
| val period: String? = null, | ||
| val time: String? = null, | ||
| val description: String? = null, | ||
| val scorer: String? = null, | ||
| val assist: String? = null, | ||
| val scoreBy: String? = null, | ||
| val corScore: Int? = null, | ||
| val oppScore: Int? = null | ||
| ) | ||
|
|
||
| fun SocketBoxScoreEntry.toGameDetailsBoxScore() = GameDetailsBoxScore( | ||
| team = team, period = period, time = time, description = description, | ||
| scorer = scorer, assist = assist, scoreBy = scoreBy, corScore = corScore, oppScore = oppScore | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,16 @@ | ||
| package com.cornellappdev.score.viewmodel | ||
|
|
||
| import androidx.lifecycle.SavedStateHandle | ||
| import androidx.navigation.toRoute | ||
| import androidx.lifecycle.viewModelScope | ||
| import com.cornellappdev.score.model.ApiResponse | ||
| import com.cornellappdev.score.model.DetailsCardData | ||
| import com.cornellappdev.score.model.ScoreRepository | ||
| import com.cornellappdev.score.model.SocketManager | ||
| import com.cornellappdev.score.model.applySocketUpdate | ||
| import com.cornellappdev.score.model.map | ||
| import com.cornellappdev.score.model.toGameCardData | ||
| import com.cornellappdev.score.nav.root.ScoreScreens | ||
| import dagger.hilt.android.lifecycle.HiltViewModel | ||
| import kotlinx.coroutines.launch | ||
| import javax.inject.Inject | ||
|
|
||
| data class GameDetailsUiState( | ||
|
|
@@ -18,6 +20,7 @@ data class GameDetailsUiState( | |
| @HiltViewModel | ||
| class GameDetailsViewModel @Inject constructor( | ||
| private val scoreRepository: ScoreRepository, | ||
| private val socketManager: SocketManager, | ||
| savedStateHandle: SavedStateHandle, | ||
| ) : BaseViewModel<GameDetailsUiState>( | ||
| initialUiState = GameDetailsUiState( | ||
|
|
@@ -37,10 +40,28 @@ class GameDetailsViewModel @Inject constructor( | |
| } | ||
| } | ||
| onRefresh() | ||
|
|
||
| socketManager.subscribe(gameId) | ||
|
|
||
| viewModelScope.launch { | ||
| socketManager.gameUpdateFlow.collect { envelope -> | ||
| if (envelope.gameId != gameId) return@collect | ||
| applyMutation { | ||
| val current = loadedState | ||
| if (current !is ApiResponse.Success) return@applyMutation this | ||
| copy(loadedState = ApiResponse.Success(current.data.applySocketUpdate(envelope.data))) | ||
|
Comment on lines
+51
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Keep matching updates while the REST request is loading.
Store the latest matching update while loading. Merge it when the next 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun onRefresh() { | ||
| applyMutation { copy(loadedState = ApiResponse.Loading) } | ||
| scoreRepository.getGameById(gameId) | ||
| } | ||
| } | ||
|
|
||
| override fun onCleared() { | ||
| super.onCleared() | ||
| socketManager.unsubscribe(gameId) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Derive totals from a score-breakdown-only update.
A valid update can contain
scoreBreakdownwhile omittinghomeScoreandoppScore. This function rebuildsgameDatafrom that breakdown, but lines 328-329 retain the old totals. The header can then disagree with the updated period rows.Use the same
convertScoresfallback used byGameDetailsGame.toGameCardData().Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents