Streaming feature#165
Merged
Merged
Conversation
|
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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a symlink-safe streaming Service, a Fiber HTTP handler and single /stream route, locator methods on importing/library services, hosting wiring and template helpers, plus frontend audio players, JS controls, and reduced HTMX polling intervals. ChangesAudio Streaming Feature
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
🤖 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 `@src/features/streaming/service.go`:
- Around line 31-32: Cache the cleaned path once and reuse it instead of calling
filepath.Clean(path) twice: compute cleanPath := filepath.Clean(path) and
replace both occurrences in the validation if (the Prefix check and the equality
check) with cleanPath; do the same refactor inside LibraryTrackStream (the
validation block around lines 44-47) to avoid redundant filepath.Clean calls and
improve efficiency.
- Around line 52-57: The mimeTypeFor function currently uses suffix matching and
misclassifies names like "file.flac.backup"; change mimeTypeFor to use
filepath.Ext to extract the actual extension, normalize it with strings.ToLower,
then switch/map on the extension (e.g., ".flac" -> "audio/flac", ".mp3" ->
"audio/mpeg") and keep a sensible default (audio/mpeg). Update references in
mimeTypeFor to import/path-qualify filepath and remove reliance on
strings.HasSuffix to make detection robust for future formats.
- Around line 30-34: The path containment check in
QueueTrackStream/LibraryTrackStream currently only uses filepath.Clean and then
passes the original path to c.SendFile, allowing symlink escapes; change the
validation to call filepath.Clean once, then call filepath.EvalSymlinks on both
the cleaned candidate path and the cleaned base (DownloadPath/LibraryPath) and
perform the containment check against the resolved base (e.g., ensure
resolvedCandidate has the resolvedBase+separator prefix or equals resolvedBase);
also eliminate repeated filepath.Clean(path) calls by reusing the cleaned value,
and tighten the MIME/extension detection logic used after validation by relying
on evaluated file extension/mime (e.g., use filepath.Ext and
mime.TypeByExtension or an explicit extension whitelist) rather than naive
HasSuffix(".flac") checks so renamed files cannot be misclassified.
In `@views/importing/queue_items_grouped_album.html`:
- Line 144: The audio progress range inputs (the <input type="range"> elements
that call seekAudioProgress(this) and use class "flex-1 h-1 rounded-full
cursor-pointer accent-[`#463371`] mx-1") need accessible labels: add an aria-label
(e.g., aria-label="Audio progress") to each of those range inputs (apply same
change to the other occurrences referenced in the comment) so screen readers can
announce the control; ensure the aria-label text describes the control (or make
it dynamic if it represents a specific track).
In `@views/importing/queue_items_grouped_artist.html`:
- Line 142: Add accessible labels to the audio progress range inputs by adding
an aria-label attribute (e.g., aria-label="Audio progress" or a more specific
label like "Track progress for {trackTitle}") to the <input type="range">
elements that call seekAudioProgress(this) (the elements with class "flex-1 h-1
rounded-full cursor-pointer accent-[`#463371`] mx-1"); apply the same aria-label
pattern to the other two range inputs referenced in the review so screen readers
can identify the sliders.
In `@views/importing/queue_items.html`:
- Line 64: Add accessible labels to the audio progress range inputs by adding an
aria-label attribute (e.g., aria-label="Audio progress" or a more specific label
like "Playback position") to each <input type="range"> that uses the
seekAudioProgress handler; update the inputs referenced (the element with
oninput="seekAudioProgress(this)" and the similar inputs at the other
occurrences) so screen readers can announce the control purpose while preserving
existing attributes (value, min, max, step, class, oninput).
In `@views/tag/edit_form.html`:
- Around line 128-131: The range input for audio progress in edit_form.html is
missing an accessible label; add an aria-label (e.g., aria-label="Audio
progress" or a localized string) to the <input type="range"> so screen readers
can identify it, keeping the existing attributes and
oninput="seekAudioProgress(this)"; ensure the label accurately describes the
control and update any related docs or tests if they rely on query selectors for
unlabeled inputs.
- Line 119: The download attribute currently uses the full path via
{{.Track.Path}} which can yield ugly absolute paths; update the anchor element
(the link with href "/stream/library/{{.Track.ID}}") to use only the base
filename for download by replacing {{.Track.Path}} with a basename extractor
(e.g. use an existing template func like {{basename .Track.Path}} or register a
helper that calls path.Base and then use {{pathBase .Track.Path}}); if no helper
exists, add one to the template funcs and use that here so the download
attribute contains only the filename (e.g. "track.flac") instead of the full
path.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c2c850f-ffa2-42a2-bd24-647bc5cb9bf2
📒 Files selected for processing (16)
src/features/hosting/server.gosrc/features/importing/service.gosrc/features/library/service.gosrc/features/streaming/handlers.gosrc/features/streaming/routes.gosrc/features/streaming/service.gosrc/features/streaming/streaming.gosrc/main.goviews/importing/queue_header.htmlviews/importing/queue_items.htmlviews/importing/queue_items_grouped_album.htmlviews/importing/queue_items_grouped_artist.htmlviews/partials/navbar.htmlviews/partials/scripts.htmlviews/partials/sidebar.htmlviews/tag/edit_form.html
| <button onclick="toggleAudioPlayer(this)" class="play-btn flex-shrink-0 w-5 h-5 flex items-center justify-center rounded-full bg-violet-500/20 hover:bg-violet-500/40 text-violet-600 dark:text-violet-300 transition-colors"> | ||
| <i class="fas fa-play fa-xs"></i> | ||
| </button> | ||
| <input type="range" oninput="seekAudioProgress(this)" class="flex-1 h-1 rounded-full cursor-pointer accent-[#463371] mx-1" value="0" min="0" max="100" step="0.1"> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add aria-label attributes to audio progress range inputs for accessibility.
The progress sliders lack accessible labels for screen reader users.
♿ Suggested accessibility fix
-<input type="range" oninput="seekAudioProgress(this)" class="flex-1 h-1 rounded-full cursor-pointer accent-[`#463371`] mx-1" value="0" min="0" max="100" step="0.1">
+<input type="range" oninput="seekAudioProgress(this)" aria-label="Audio progress" class="flex-1 h-1 rounded-full cursor-pointer accent-[`#463371`] mx-1" value="0" min="0" max="100" step="0.1">Apply the same pattern to lines 150 and 165.
Also applies to: 150-150, 165-165
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 142-142: No matching [ label ] tag found.
(input-requires-label)
🤖 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 `@views/importing/queue_items_grouped_artist.html` at line 142, Add accessible
labels to the audio progress range inputs by adding an aria-label attribute
(e.g., aria-label="Audio progress" or a more specific label like "Track progress
for {trackTitle}") to the <input type="range"> elements that call
seekAudioProgress(this) (the elements with class "flex-1 h-1 rounded-full
cursor-pointer accent-[`#463371`] mx-1"); apply the same aria-label pattern to the
other two range inputs referenced in the review so screen readers can identify
the sliders.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@views/library/track_overview_panel.html`:
- Around line 33-35: The audio element in the template lacks an error handler so
stream load failures provide no user feedback; add an onerror attribute (e.g.
onerror="handleAudioError(this, event)") to the <audio> tag and implement a
corresponding handler function (handleAudioError) that inspects the event,
displays a user-facing message or hides/cleans up the player, and logs the
error; hook this handler alongside the existing updateAudioProgress(this) and
resetAudioPlayer(this) usage so failures are handled consistently with those
functions.
- Line 29: The range input in track_overview_panel.html (the <input type="range"
min="0" max="100" value="0" step="0.1"> element) is missing an accessible label;
add an aria-label attribute to that input (for example aria-label="Track level"
or an appropriate description like "Track volume" or "Track gain") so screen
readers can announce the control's purpose, e.g. update the input element to
include aria-label="..." matching the UI semantics.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4147cc03-1ad1-4610-ad3c-ef13530f51c4
📒 Files selected for processing (2)
views/library/track_overview_panel.htmlviews/partials/scripts.html
💤 Files with no reviewable changes (1)
- views/partials/scripts.html
Comment on lines
+33
to
+35
| <audio src="/stream/library/{{.Track.ID}}" preload="none" | ||
| ontimeupdate="updateAudioProgress(this)" | ||
| onended="resetAudioPlayer(this)"></audio> |
There was a problem hiding this comment.
Add error handler for audio loading failures.
The <audio> element has no onerror handler. If the stream fails to load (network error, file not found, unsupported format), users receive no feedback. Consider adding an onerror handler to provide visual feedback or gracefully hide the player.
🛡️ Proposed fix
<audio src="/stream/library/{{.Track.ID}}" preload="none"
ontimeupdate="updateAudioProgress(this)"
- onended="resetAudioPlayer(this)"></audio>
+ onended="resetAudioPlayer(this)"
+ onerror="this.closest('.audio-player').style.display='none'"></audio>📝 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.
Suggested change
| <audio src="/stream/library/{{.Track.ID}}" preload="none" | |
| ontimeupdate="updateAudioProgress(this)" | |
| onended="resetAudioPlayer(this)"></audio> | |
| <audio src="/stream/library/{{.Track.ID}}" preload="none" | |
| ontimeupdate="updateAudioProgress(this)" | |
| onended="resetAudioPlayer(this)" | |
| onerror="this.closest('.audio-player').style.display='none'"></audio> |
🤖 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 `@views/library/track_overview_panel.html` around lines 33 - 35, The audio
element in the template lacks an error handler so stream load failures provide
no user feedback; add an onerror attribute (e.g. onerror="handleAudioError(this,
event)") to the <audio> tag and implement a corresponding handler function
(handleAudioError) that inspects the event, displays a user-facing message or
hides/cleans up the player, and logs the error; hook this handler alongside the
existing updateAudioProgress(this) and resetAudioPlayer(this) usage so failures
are handled consistently with those functions.
contre95
commented
Jun 7, 2026
| } | ||
|
|
||
| // StreamQueueItem streams a pending track from the download folder. | ||
| func (h *Handler) StreamQueueItem(c *fiber.Ctx) error { |
Owner
Author
There was a problem hiding this comment.
why queue item, just stream a son.Either from path or from library
| // Service handles audio streaming from both the download folder and the library. | ||
| type Service struct { | ||
| queue QueueLocator | ||
| library LibraryLocator |
Owner
Author
There was a problem hiding this comment.
why locator? shouldn't this be on the domain? or referenced from it ?
| } | ||
|
|
||
| // QueueTrackStream returns the validated file path and MIME type for a pending queue item. | ||
| func (s *Service) QueueTrackStream(itemID string) (string, string, error) { |
Owner
Author
There was a problem hiding this comment.
Same comment as above. it shouls either be downñpad path or library. Queue items are just a consequence of that use case that should be handled by the handlers
| @@ -0,0 +1,13 @@ | |||
| package streaming | |||
Owner
Author
There was a problem hiding this comment.
This entire file is probably not needed. We can already get the file from the library service or the importing service
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.go (1)
139-151:⚠️ Potential issue | 🟠 MajorFix server lifecycle:
Start()is blocking, so the signal/shutdown wiring is currently unreachable andStart()is effectively started twice
server.Start()delegates to Fiber’sapp.Listen(...)(blocking). The synchronous call atsrc/main.golines ~139-142 prevents the code from ever reachingsignal.Notify(...), theselect { ... }, andserver.Shutdown()(lines ~143-166). The goroutineserver.Start()(lines ~146-151) only runs if the firstStart()returns immediately with an error, and can then cause redundant/double bind attempts.🤖 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 `@src/main.go` around lines 139 - 151, The current code calls server.Start() synchronously then again in a goroutine, causing blocking and double-start issues; remove the initial synchronous call and start the server exactly once inside the goroutine that writes errors to serverErr (keep the serverErr channel and quit channel created), call signal.Notify(quit, os.Interrupt) before launching the goroutine, then use a select to wait on quit or serverErr and call server.Shutdown() on shutdown path; update logging to reflect single startup via slog.Info in the goroutine and ensure any error returned from server.Start() is forwarded to serverErr for the select to handle.
🤖 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 `@src/features/streaming/handlers.go`:
- Around line 22-24: The code double-decodes the query param by calling
url.QueryUnescape on c.Query("path"); remove the url.QueryUnescape call and the
net/url import, use the value returned by c.Query("path") directly (e.g.,
replace rawPath/path logic with a single value from c.Query("path")), and delete
the now-unnecessary err handling around QueryUnescape so streaming lookups use
the original decoded query string.
In `@src/features/streaming/service.go`:
- Around line 40-49: The Stream method currently allows any file under allowed
directories and relies on mimeTypeFor to default unknown extensions to
audio/mpeg; change Stream (Service.Stream) to enforce an explicit audio
extension allowlist (e.g., .mp3, .wav, .flac, .aac, .m4a, etc.) after resolving
the path with containedIn, and return an error if the file extension is not in
the allowlist; update callers or mimeTypeFor usage so mimeTypeFor is only
invoked for allowed extensions and do not treat unknown extensions as
audio/mpeg.
---
Outside diff comments:
In `@src/main.go`:
- Around line 139-151: The current code calls server.Start() synchronously then
again in a goroutine, causing blocking and double-start issues; remove the
initial synchronous call and start the server exactly once inside the goroutine
that writes errors to serverErr (keep the serverErr channel and quit channel
created), call signal.Notify(quit, os.Interrupt) before launching the goroutine,
then use a select to wait on quit or serverErr and call server.Shutdown() on
shutdown path; update logging to reflect single startup via slog.Info in the
goroutine and ensure any error returned from server.Start() is forwarded to
serverErr for the select to handle.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 44a9f0a1-85a2-4d95-8d6e-24e61a74097d
📒 Files selected for processing (11)
src/features/hosting/server.gosrc/features/streaming/handlers.gosrc/features/streaming/routes.gosrc/features/streaming/service.gosrc/features/streaming/streaming.gosrc/main.goviews/importing/queue_items.htmlviews/importing/queue_items_grouped_album.htmlviews/importing/queue_items_grouped_artist.htmlviews/library/track_overview_panel.htmlviews/tag/edit_form.html
💤 Files with no reviewable changes (1)
- src/features/streaming/streaming.go
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Note
This PR introduces a streaming feature. This feature intends not to turn Soulsolid into a player but to help during the tasks as figuring out if a song is the same as the other for duplicates queue items or playing a song when being preview/edited.
validating files stay within their allowed directories
(download folder) and existing (library) file before deciding to replace or skip
immediately on user action
Summary by CodeRabbit
New Features
Performance
UI/UX