[CUS-11053] Addon for playing the mp3 audio in background using MP3 SPI#351
[CUS-11053] Addon for playing the mp3 audio in background using MP3 SPI#351ManojTestsigma wants to merge 1 commit intodevfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new Maven module Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@audio_file_actions/src/main/java/com/testsigma/addons/util/AudioPlaybackUtil.java`:
- Around line 23-25: The extractFileName method can NPE when
urlOrPath/fileName/url are null or blank; add a null/blank guard at the start of
extractFileName (e.g., if urlOrPath == null || urlOrPath.trim().isEmpty() throw
new IllegalArgumentException("urlOrPath must not be null or blank")), then trim
the value and only perform startsWith("http://"/"https://") and URL parsing
after that check; also apply the same defensive checks before using local
variables named fileName and url in any helper methods or branches so you return
or throw a controlled validation error instead of allowing an NPE.
- Around line 57-58: The temp-file suffix extraction incorrectly splits fileName
on every '.' which fails for names like "song.final.mp3"; update the temp file
creation in AudioPlaybackUtil (the File tempFile = File.createTempFile(...) call
using fileName) to determine baseName = fileName.substring(0,
fileName.lastIndexOf('.')) and suffix =
fileName.substring(fileName.lastIndexOf('.')) (or use a safe fallback when
lastIndexOf('.') == -1, e.g., suffix=".tmp") so the suffix includes the dot and
works for multi-dot filenames; ensure you pass baseName and suffix into
File.createTempFile and handle the no-extension case.
- Around line 54-60: The current remote-download block in AudioPlaybackUtil (the
if branch that creates urlObject and tempFile) calls
FileUtils.copyURLToFile(urlObject, tempFile) without timeouts; replace that call
with the Commons-IO overload FileUtils.copyURLToFile(URL, File, int
connectionTimeoutMillis, int readTimeoutMillis) and pass sensible timeout values
(e.g., connectionTimeout 10000, readTimeout 60000) or read them from
configuration/constants; keep using the same urlObject and tempFile variables
and ensure any IOException handling remains unchanged.
- Around line 76-80: The playMp3 method currently calls Player.play() on the
caller thread which blocks until playback completes; change playMp3 to perform
playback asynchronously by creating a dedicated Thread or using an
ExecutorService to run the Player.play() call so the method returns immediately
and playback happens in background; ensure the FileInputStream and Player are
created inside the runnable and properly closed (or use try-with-resources
inside the task) and handle/rethrow exceptions from the background task if
needed; update references in playMp3 and any callers to account for non-blocking
behavior.
In `@audio_file_actions/src/main/java/com/testsigma/addons/web/PlayAudio.java`:
- Line 29: In PlayAudio.java where you read String inputValue =
filePath.getValue().toString(); add null checks for filePath and
filePath.getValue() before calling toString() (e.g., if filePath == null ||
filePath.getValue() == null) and handle the missing input by throwing or
returning a clear validation error/message (or logging and failing the step) so
the code does not NPE when test data is absent; update the validation path in
the same method that uses filePath to produce the user-facing error instead of
letting toString() throw.
In `@audio_file_actions/src/main/resources/testsigma-sdk.properties`:
- Line 1: The file contains a committed plaintext secret for
testsigma-sdk.api.key; remove the hardcoded token from testsigma-sdk.properties,
replace it with a non-secret placeholder (e.g. testsigma-sdk.api.key=REDACTED)
or an environment variable reference, and update the code that reads
testsigma-sdk.api.key to load the value from a secure source (environment
variable or secrets manager) instead of the properties file; after removing the
secret from VCS, immediately revoke/rotate the exposed API key in the provider
console.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffa6de51-fea8-4bde-815c-f34786c5d46c
📒 Files selected for processing (5)
audio_file_actions/pom.xmlaudio_file_actions/src/main/java/com/testsigma/addons/android/PlayAudio.javaaudio_file_actions/src/main/java/com/testsigma/addons/util/AudioPlaybackUtil.javaaudio_file_actions/src/main/java/com/testsigma/addons/web/PlayAudio.javaaudio_file_actions/src/main/resources/testsigma-sdk.properties
| public String extractFileName(String urlOrPath) { | ||
| String path = urlOrPath; | ||
| if (urlOrPath.startsWith("http://") || urlOrPath.startsWith("https://")) { |
There was a problem hiding this comment.
Guard null/blank input before parsing and URL checks.
urlOrPath, fileName, or url can be null/blank and currently trigger NPEs (Line 25, Line 42, Line 54) instead of controlled validation errors.
Suggested fix
+import java.util.Objects;
...
public String extractFileName(String urlOrPath) {
+ Objects.requireNonNull(urlOrPath, "urlOrPath cannot be null");
+ if (urlOrPath.isBlank()) {
+ throw new IllegalArgumentException("urlOrPath cannot be blank");
+ }
String path = urlOrPath;
...
public void validateFormat(String fileName) {
+ Objects.requireNonNull(fileName, "fileName cannot be null");
if (!fileName.toLowerCase().endsWith(".mp3")) {
...
public File urlToFileConverter(String fileName, String url) {
+ Objects.requireNonNull(url, "url cannot be null");
+ if (url.isBlank()) {
+ throw new IllegalArgumentException("url cannot be blank");
+ }Also applies to: 41-43, 52-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@audio_file_actions/src/main/java/com/testsigma/addons/util/AudioPlaybackUtil.java`
around lines 23 - 25, The extractFileName method can NPE when
urlOrPath/fileName/url are null or blank; add a null/blank guard at the start of
extractFileName (e.g., if urlOrPath == null || urlOrPath.trim().isEmpty() throw
new IllegalArgumentException("urlOrPath must not be null or blank")), then trim
the value and only perform startsWith("http://"/"https://") and URL parsing
after that check; also apply the same defensive checks before using local
variables named fileName and url in any helper methods or branches so you return
or throw a controlled validation error instead of allowing an NPE.
| if (url.startsWith("https://") || url.startsWith("http://")) { | ||
| logger.info("Given is s3 url ...File name: " + fileName); | ||
| URL urlObject = new URL(url); | ||
| File tempFile = File.createTempFile(fileName.split("\\.")[0], | ||
| "." + fileName.split("\\.")[1]); | ||
| FileUtils.copyURLToFile(urlObject, tempFile); | ||
| logger.info("Temp file created: " + tempFile.getName() + " at " + tempFile.getAbsolutePath()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n audio_file_actions/src/main/java/com/testsigma/addons/util/AudioPlaybackUtil.javaRepository: testsigmahq/testsigma-addons
Length of output: 1875
🌐 Web query:
Apache Commons IO FileUtils copyURLToFile timeout parameters overload
💡 Result:
FileUtils.copyURLToFile has an overload that lets you set timeouts:
FileUtils.copyURLToFile(URL source,
File destination,
int connectionTimeoutMillis,
int readTimeoutMillis);connectionTimeoutMillis: max time (ms) to establish the connectionreadTimeoutMillis: max time (ms) waiting for data while reading (per read)- The no-timeout overload
copyURLToFile(URL, File)warns it may “block forever”; use the timeout overload instead. [1]
This timeout overload was added to Commons IO as part of issue IO-208 and shipped in commons-io 2.0. [2]
Sources: Apache Commons IO FileUtils API docs [1]; Apache Commons IO JIRA IO-208 [2].
Add connection/read timeouts for remote downloads.
Line 59 uses FileUtils.copyURLToFile(urlObject, tempFile) which lacks timeouts. The no-timeout overload can block indefinitely on network stalls. Commons IO provides a 4-parameter overload to specify connection and read timeouts; use it instead.
Suggested fix
- FileUtils.copyURLToFile(urlObject, tempFile);
+ FileUtils.copyURLToFile(urlObject, tempFile, 10_000, 30_000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@audio_file_actions/src/main/java/com/testsigma/addons/util/AudioPlaybackUtil.java`
around lines 54 - 60, The current remote-download block in AudioPlaybackUtil
(the if branch that creates urlObject and tempFile) calls
FileUtils.copyURLToFile(urlObject, tempFile) without timeouts; replace that call
with the Commons-IO overload FileUtils.copyURLToFile(URL, File, int
connectionTimeoutMillis, int readTimeoutMillis) and pass sensible timeout values
(e.g., connectionTimeout 10000, readTimeout 60000) or read them from
configuration/constants; keep using the same urlObject and tempFile variables
and ensure any IOException handling remains unchanged.
| File tempFile = File.createTempFile(fileName.split("\\.")[0], | ||
| "." + fileName.split("\\.")[1]); |
There was a problem hiding this comment.
Fix temp-file suffix extraction for filenames with multiple dots.
Line 57-58 assumes exactly one . in the file name. For values like song.final.mp3, suffix becomes incorrect and can fail or mis-handle temp file naming.
Suggested fix
- File tempFile = File.createTempFile(fileName.split("\\.")[0],
- "." + fileName.split("\\.")[1]);
+ int dotIndex = fileName.lastIndexOf('.');
+ String baseName = (dotIndex > 0) ? fileName.substring(0, dotIndex) : fileName;
+ String extension = (dotIndex > 0) ? fileName.substring(dotIndex) : ".mp3";
+ String safePrefix = (baseName.length() >= 3) ? baseName : "aud";
+ File tempFile = File.createTempFile(safePrefix, extension);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| File tempFile = File.createTempFile(fileName.split("\\.")[0], | |
| "." + fileName.split("\\.")[1]); | |
| int dotIndex = fileName.lastIndexOf('.'); | |
| String baseName = (dotIndex > 0) ? fileName.substring(0, dotIndex) : fileName; | |
| String extension = (dotIndex > 0) ? fileName.substring(dotIndex) : ".mp3"; | |
| String safePrefix = (baseName.length() >= 3) ? baseName : "aud"; | |
| File tempFile = File.createTempFile(safePrefix, extension); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@audio_file_actions/src/main/java/com/testsigma/addons/util/AudioPlaybackUtil.java`
around lines 57 - 58, The temp-file suffix extraction incorrectly splits
fileName on every '.' which fails for names like "song.final.mp3"; update the
temp file creation in AudioPlaybackUtil (the File tempFile =
File.createTempFile(...) call using fileName) to determine baseName =
fileName.substring(0, fileName.lastIndexOf('.')) and suffix =
fileName.substring(fileName.lastIndexOf('.')) (or use a safe fallback when
lastIndexOf('.') == -1, e.g., suffix=".tmp") so the suffix includes the dot and
works for multi-dot filenames; ensure you pass baseName and suffix into
File.createTempFile and handle the no-extension case.
| public void playMp3(File audioFile) throws Exception { | ||
| logger.info("Using JLayer for MP3 playback: " + audioFile.getName()); | ||
| try (FileInputStream fis = new FileInputStream(audioFile)) { | ||
| Player player = new Player(fis); | ||
| player.play(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
javazoom.jl.player.Player.play() blocking behavior thread documentation
💡 Result:
javazoom.jl.player.Player.play() is blocking: it runs decoding/playback synchronously in the calling thread until the stream ends (or you stop it).
Evidence:
Player.play()simply delegates toplay(Integer.MAX_VALUE), andplay(int frames)loops (while (frames-- > 0 && ret) { ret = decodeFrame(); }) until decoding finishes, then flushes/closes the audio device—i.e., it does not return until playback is done (or fails/stops). [1]- Practical guidance commonly given for JLayer is therefore: “the JLayer
play()method runs entirely in the calling thread … it blocks until the song is complete. So run in its own thread.” [2] - The JLayer distribution also distinguishes a “simple” command-line player vs an “advanced (threaded) MP3 player” (
javazoom.jl.player.advanced.jlap), reinforcing that basicPlayerplayback is not threaded by default. [3]
Implication: if you don’t want to block (e.g., UI thread), call player.play() from a dedicated thread/executor.
Sources: [1] https://github.com/wkpark/JLayer/blob/master/src/javazoom/jl/player/Player.java [2] https://www.javatips.net/api/javazoom.jl.player.advanced.advancedplayer [3] https://github.com/umjammer/jlayer
🏁 Script executed:
# First, let's look at the complete context of the AudioPlaybackUtil.java file
find . -name "AudioPlaybackUtil.java" -type fRepository: testsigmahq/testsigma-addons
Length of output: 1875
🏁 Script executed:
# Search for any related code or comments about background playback requirements
rg -i "background.*playback|playback.*background" --type javaRepository: testsigmahq/testsigma-addons
Length of output: 1875
player.play() blocks the calling thread until playback completes, preventing background playback behavior.
JLayer's Player.play() method runs synchronously and does not return until the full audio file is decoded and played. Per JLayer documentation and best practices, running this on the calling thread conflicts with background playback requirements. To achieve non-blocking behavior, wrap the call in a separate thread or use an executor service.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@audio_file_actions/src/main/java/com/testsigma/addons/util/AudioPlaybackUtil.java`
around lines 76 - 80, The playMp3 method currently calls Player.play() on the
caller thread which blocks until playback completes; change playMp3 to perform
playback asynchronously by creating a dedicated Thread or using an
ExecutorService to run the Player.play() call so the method returns immediately
and playback happens in background; ensure the FileInputStream and Player are
created inside the runnable and properly closed (or use try-with-resources
inside the task) and handle/rethrow exceptions from the background task if
needed; update references in playMp3 and any callers to account for non-blocking
behavior.
| protected Result execute() throws NoSuchElementException { | ||
| Result result = Result.SUCCESS; | ||
| try { | ||
| String inputValue = filePath.getValue().toString(); |
There was a problem hiding this comment.
Null-check filePath and its value before toString().
Line 29 can throw NPE when test data is missing, which bypasses clear input-validation messaging.
Suggested fix
- String inputValue = filePath.getValue().toString();
+ if (filePath == null || filePath.getValue() == null) {
+ throw new IllegalArgumentException("file-path-or-url is required");
+ }
+ String inputValue = filePath.getValue().toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@audio_file_actions/src/main/java/com/testsigma/addons/web/PlayAudio.java` at
line 29, In PlayAudio.java where you read String inputValue =
filePath.getValue().toString(); add null checks for filePath and
filePath.getValue() before calling toString() (e.g., if filePath == null ||
filePath.getValue() == null) and handle the missing input by throwing or
returning a clear validation error/message (or logging and failing the step) so
the code does not NPE when test data is absent; update the validation path in
the same method that uses filePath to produce the user-facing error instead of
letting toString() throw.
| @@ -0,0 +1 @@ | |||
| testsigma-sdk.api.key=eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiIyNDBkMjhiNS0xNmUwLThlNmYtOWQ0ZS05MjYxMGNiZTcyYzciLCJ1bmlxdWVJZCI6IjYwMjEiLCJpZGVudGl0eUFjY291bnRVVUlkIjoiMzUifQ.de7OAX_uP4IlafBX3zKctJ9FsqzQuVZGqpvUwZNTmBL2_ZDB-ovxX42HFMjfs9A5-uccLvw0cKSRX0i6wJ2KWQ No newline at end of file | |||
There was a problem hiding this comment.
Remove committed API key immediately and rotate the credential.
Line 1 contains a live-looking API token in plaintext. This is a critical secret-leak risk and should not be stored in VCS.
Suggested fix
-testsigma-sdk.api.key=eyJhbGciOiJIUzUxMiJ9...
+testsigma-sdk.api.key=${TESTSIGMA_SDK_API_KEY}Please also revoke/rotate the exposed key in the backing system.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testsigma-sdk.api.key=eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiIyNDBkMjhiNS0xNmUwLThlNmYtOWQ0ZS05MjYxMGNiZTcyYzciLCJ1bmlxdWVJZCI6IjYwMjEiLCJpZGVudGl0eUFjY291bnRVVUlkIjoiMzUifQ.de7OAX_uP4IlafBX3zKctJ9FsqzQuVZGqpvUwZNTmBL2_ZDB-ovxX42HFMjfs9A5-uccLvw0cKSRX0i6wJ2KWQ | |
| testsigma-sdk.api.key=${TESTSIGMA_SDK_API_KEY} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@audio_file_actions/src/main/resources/testsigma-sdk.properties` at line 1,
The file contains a committed plaintext secret for testsigma-sdk.api.key; remove
the hardcoded token from testsigma-sdk.properties, replace it with a non-secret
placeholder (e.g. testsigma-sdk.api.key=REDACTED) or an environment variable
reference, and update the code that reads testsigma-sdk.api.key to load the
value from a secure source (environment variable or secrets manager) instead of
the properties file; after removing the secret from VCS, immediately
revoke/rotate the exposed API key in the provider console.
2 participants
please review this addon and publish as PUBLIC
Addon name : audio_file_actions
Addon accont: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira: https://testsigma.atlassian.net/browse/CUS-11053
fix
Summary by CodeRabbit