feat(subsonic): implement indexBasedQueue extension #4244
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @kgarner7, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces support for the indexBasedQueue OpenSubsonic extension, enhancing play queue management by allowing the current track to be specified by an optional index. This involves significant changes to the data model, persistence layer, and API endpoints to accommodate the new optional index, along with a corresponding database migration.
Highlights
- Subsonic Extension Implementation: I've implemented the
indexBasedQueueOpenSubsonic extension, which allows clients to manage play queues using an index to specify the current track. - Optional Current Index: The
Currentfield in thePlayQueuemodel and its associated persistence layer has been updated from anintto a pointer*int, making it optional (nullable) in the database and Go structures. - New API Endpoints: I've introduced two new Subsonic API endpoints:
getPlayQueueByIndexandsavePlayQueueByIndex, to facilitate the new index-based queue management functionality. - Database Migration: A new database migration is included to modify the
playqueuetable, specifically making thecurrentcolumn optional to align with the Go model changes.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request implements the indexBasedQueue extension for Subsonic, allowing the current track in a play queue to be optional and identified by its index. The changes span database migrations, model updates, persistence layer adjustments, and new API endpoints, along with corresponding test updates.
My main review points focus on ensuring the database migration is fully reversible by implementing a proper Down script, and improving an error message in one of the new API handlers for better clarity to API consumers. Addressing these points will enhance the robustness and maintainability of this feature.
| -- +goose Down | ||
| -- +goose StatementBegin | ||
| SELECT 'down SQL query'; | ||
| -- +goose StatementEnd |
There was a problem hiding this comment.
The Down migration currently contains a placeholder and does not revert the schema changes made in the Up migration. It should restore the current column to its previous state. Implement the Down migration to accurately reverse the Up operations.
-- +goose Down
-- +goose StatementBegin
-- Assuming the previous state was 'current INTEGER NOT NULL DEFAULT 0'
-- Adjust if the original constraints were different.
ALTER TABLE playqueue ADD COLUMN current_old INTEGER NOT NULL DEFAULT 0;
UPDATE playqueue SET current_old = COALESCE(current, 0);
ALTER TABLE playqueue DROP COLUMN current;
ALTER TABLE playqueue RENAME COLUMN current_old TO current;
-- +goose StatementEnd| index = ¤tIndex | ||
|
|
||
| if currentIndex < 0 || currentIndex >= len(items) { | ||
| return nil, newError(responses.ErrorMissingParameter, "missing parameter index, err: %s", err) |
There was a problem hiding this comment.
The error message used when currentIndex is out of bounds could be more precise. Consider using responses.ErrorDataNotFound or a more generic error indicating an invalid value, along with a clearer message.
| return nil, newError(responses.ErrorMissingParameter, "missing parameter index, err: %s", err) | |
| return nil, newError(responses.ErrorDataNotFound, "currentIndex out of bounds") |
| if pq.Current != nil { | ||
| current := *pq.Current | ||
| if current >= 0 && current < len(pq.Items) { | ||
| currentID = pq.Items[current].ID | ||
| } | ||
| } |
There was a problem hiding this comment.
This block handles the case where pq.Current is nil, preventing a panic when dereferencing a nil pointer.
No description provided.