fix: added null check for mediaNotificationControllerInfo in BaseSess…#752
fix: added null check for mediaNotificationControllerInfo in BaseSess…#752eddyizm wants to merge 2 commits into
Conversation
…ionCallback.updateMediaNotificationCustomLayout
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/build.gradle (1)
137-140: Validate security/compatibility of added test deps
junit:junit:4.13.2is outside the reported vulnerable range (>= 4.7, < 4.13.1); no advisory for4.13.2.- No security advisories were returned for
org.mockito:mockito-core:5.12.0ororg.mockito.kotlin:mockito-kotlin:5.4.0.mockito-kotlin:5.4.0is compatible withmockito-core:5.12.0(it declares that dependency).- Optional upgrade: Mockito core has a newer 5.x (
5.23.0); upgrading may require coordinating themockito-kotlinversion (latest major is 6.x).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/build.gradle` around lines 137 - 140, The added test dependencies (junit:junit:4.13.2, org.mockito:mockito-core:5.12.0, org.mockito.kotlin:mockito-kotlin:5.4.0) are not flagged as vulnerable but Mockito core has a newer 5.x (5.23.0); either explicitly upgrade org.mockito:mockito-core to 5.23.0 and bump org.mockito.kotlin:mockito-kotlin to a mutually compatible release (ensure mockito-kotlin declares or supports that mockito-core version), or keep the current coordinates but add a short comment in build.gradle explaining the versions were validated against advisories and are intentionally retained; reference the dependency coordinates to locate and change the declarations.app/src/test/java/com/cappielloantonio/tempo/service/BaseSessionCallbackTest.kt (1)
70-87: ⚡ Quick winAdd test coverage for
handlePlayerChangedcalled beforeonConnect.The existing test verifies that
handlePlayerChangedmoves the listener between players, but it doesn't cover the initialization scenario wherehandlePlayerChangedmight be called before any controller has connected (i.e., beforeonConnectsetscurrentSession).Add a test to verify that
handlePlayerChangeddoes not add the listener whencurrentSessionisnull:🧪 Suggested test
`@Test` fun handlePlayerChanged_doesNotAddListenerBeforeOnConnect() { val context = mock<Context>() val service = mock<BaseMediaService>() val player = mock<Player>() whenever(context.getString(anyInt())).thenReturn("mock_string") mockConstruction(SessionCommand::class.java).use { val callback = BaseSessionCallback(context, service) // Call handlePlayerChanged before onConnect (currentSession is null) callback.handlePlayerChanged(null, player) // Should NOT add listener yet verify(player, times(0)).addListener(any()) } }This test will pass once the fix for
handlePlayerChanged(returning early whencurrentSession == null) is applied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/cappielloantonio/tempo/service/BaseSessionCallbackTest.kt` around lines 70 - 87, handlePlayerChanged currently moves listeners between players even when no controller session exists; update BaseSessionCallback.handlePlayerChanged to return early if currentSession is null (i.e., don't add a listener to the newPlayer when no session has been set by onConnect), while still safely removing a listener from oldPlayer if provided; reference the BaseSessionCallback.handlePlayerChanged method and the currentSession field and ensure onConnect is the only place that sets currentSession before listeners are added.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/cappielloantonio/tempo/service/BaseSessionCallback.kt`:
- Around line 157-160: handlePlayerChanged currently always adds playerListener
to newPlayer causing duplicate registrations when invoked before a controller
connects; change handlePlayerChanged (in BaseSessionCallback) to only move the
listener if currentSession is non-null (i.e., a controller has connected):
always remove playerListener from oldPlayer (if non-null), but only call
newPlayer.addListener(playerListener) when currentSession != null (so onConnect
remains responsible for the initial add to session.player). Reference symbols:
handlePlayerChanged, currentSession, onConnect, playerListener.
---
Nitpick comments:
In `@app/build.gradle`:
- Around line 137-140: The added test dependencies (junit:junit:4.13.2,
org.mockito:mockito-core:5.12.0, org.mockito.kotlin:mockito-kotlin:5.4.0) are
not flagged as vulnerable but Mockito core has a newer 5.x (5.23.0); either
explicitly upgrade org.mockito:mockito-core to 5.23.0 and bump
org.mockito.kotlin:mockito-kotlin to a mutually compatible release (ensure
mockito-kotlin declares or supports that mockito-core version), or keep the
current coordinates but add a short comment in build.gradle explaining the
versions were validated against advisories and are intentionally retained;
reference the dependency coordinates to locate and change the declarations.
In
`@app/src/test/java/com/cappielloantonio/tempo/service/BaseSessionCallbackTest.kt`:
- Around line 70-87: handlePlayerChanged currently moves listeners between
players even when no controller session exists; update
BaseSessionCallback.handlePlayerChanged to return early if currentSession is
null (i.e., don't add a listener to the newPlayer when no session has been set
by onConnect), while still safely removing a listener from oldPlayer if
provided; reference the BaseSessionCallback.handlePlayerChanged method and the
currentSession field and ensure onConnect is the only place that sets
currentSession before listeners are added.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a06edb33-c790-4667-abbd-b436d7d9530d
📒 Files selected for processing (5)
app/build.gradleapp/src/main/java/com/cappielloantonio/tempo/service/BaseMediaService.ktapp/src/main/java/com/cappielloantonio/tempo/service/BaseSessionCallback.ktapp/src/tempus/java/com/cappielloantonio/tempo/service/MediaService.ktapp/src/test/java/com/cappielloantonio/tempo/service/BaseSessionCallbackTest.kt
| compileOptions { | ||
| sourceCompatibility JavaVersion.VERSION_1_8 | ||
| targetCompatibility JavaVersion.VERSION_1_8 | ||
| sourceCompatibility JavaVersion.VERSION_17 | ||
| targetCompatibility JavaVersion.VERSION_17 | ||
| } |
There was a problem hiding this comment.
I vaguely remember reading about this section when creating a new project. Unlike the kotlinOptions from below, this has to do with the compatibility of some libraries.
What worries me is that this change may bump the minSdk a few versions.
There was a problem hiding this comment.
For Java, "compatibility" just means "unwrap this nice method into the most rudimentary, backward-compatible byte code for the JVM."
If nothing happens, we are all good. But if something behaves strange or starts complaining about compatibility, this may be the reason.
There was a problem hiding this comment.
Yeah it concerned me too but it was needed in order to implement the testing framework. I could have went down the wrong path as I am not very familiar with java testing myself.
This seemed to be the easiest path forward, tests succeed and I was able to do a successful build sequence. Currently testing this PR. A merge into dev and pre-release would be the next step to have more testers.
I am open to other options if changing this was the wrong way to get the tests working.
./gradlew test
I will also open another PR to add the tests to the release workflows as well.
2 participants
resolves #751
where a player event fires while no notification controller is connected.
Summary by CodeRabbit
Bug Fixes
Tests
Chores