Fix multiple bugs discovered during Playwright testing#72
Fix multiple bugs discovered during Playwright testing#72gudarzi merged 5 commits intogudarzi:v3.0from
Conversation
Bug gudarzi#4 (Critical): Spotify Search crash - Add try-catch error handling in SpotifySearchService.cs - Add Error property to SpotifySearchResult - Display errors via MudAlert in SpotifySearch.razor Bug gudarzi#1: Incorrect file extension (.exe for binary data) - Change application/octet-stream mapping from .exe to .bin - Generic binary data should not default to executable extension Bug gudarzi#3: Autostart Paused Downloads not working reliably - Call StartPausedDownloads() when downloads finish/fail - Call StartPausedDownloads() when restoring from persisted state Bug gudarzi#5: Proxy Server setting persists unexpectedly - Change ProxyUrl default from "http://localhost:8086" to empty string - Persist ProxyUrl in PersistItems() and restore in OnInitializedAsync Test project fix: - Update Microsoft.AspNetCore.Identity.EntityFrameworkCore 8.0.22 to 8.0.23 - Remove duplicate Microsoft.EntityFrameworkCore.InMemory reference
Summary of ChangesHello @thromel, 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 addresses several critical and minor bugs identified during Playwright testing, significantly improving the application's stability, user experience, and data handling. It ensures that the Spotify search feature gracefully handles API errors, corrects file extension inference for generic binary data, enhances the reliability of autostarting paused downloads, and properly manages the persistence of proxy server settings. Additionally, it includes a minor update to test project dependencies for consistency. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses several bugs discovered during testing, including a critical crash in the Spotify search feature. The fixes are well-implemented, particularly the error handling in SpotifySearchService and the security improvement of changing the default extension for binary streams. The changes for autostarting downloads and persisting the proxy URL also correctly solve the described issues. I have one suggestion to further improve the reliability of the autostart feature and enhance code quality.
| if (newStatus == EQueueItemStatus.Finished.ToString() || | ||
| newStatus == EQueueItemStatus.Paused.ToString()) | ||
| { | ||
| await StartPausedDownloads(); | ||
| } |
There was a problem hiding this comment.
Using direct enum comparison is more robust and less error-prone than comparing strings, especially if enum member names change in the future. Since item.Status is already parsed into an enum on line 370, it's better to use it for the check.
Additionally, the Cancelled status seems to be missing from this condition. A cancelled download also frees up a concurrency slot, so triggering StartPausedDownloads() in this case would make the autostart feature more reliable and consistent.
if (item.Status == EQueueItemStatus.Finished ||
item.Status == EQueueItemStatus.Paused ||
item.Status == EQueueItemStatus.Cancelled)
{
await StartPausedDownloads();
}
There was a problem hiding this comment.
- ✅ Changed to direct enum comparison using
item.Statusinstead of string comparison - ✅ Added
EQueueItemStatus.Cancelledto the condition
Updated in commit 5a51a3c.
CI Status ClarificationThe CI failures are not due to code issues - they're caused by a GitHub Actions permissions limitation with fork PRs. What Happened
Root CauseThe Test job failed at the "Comment coverage on PR" step with: This is a known GitHub limitation where workflows triggered by
Local VerificationAll 128 unit tests pass locally: Suggested Fix for WorkflowTo allow coverage comments on fork PRs, consider using - name: Comment coverage on PR
if: github.event.pull_request.head.repo.full_name == github.repository
# ... rest of stepThe code changes in this PR are correct and ready for review. |
- Use direct enum comparison instead of string comparison (more robust) - Add Cancelled status to autostart trigger (cancelled downloads also free slots)
SpotifySearch.razor: - Show error OR results, not both (prevents confusing UX) SpotifySearchService.cs: - Add ILogger for proper error logging with stack traces - Add TaskCanceledException handling (timeout vs cancellation) - Map HTTP status codes to user-friendly messages (404, 429, 503) - Log all errors with full context before returning DownloadVideoAudio.razor: - Use Enum.TryParse instead of Enum.Parse (prevents crashes on invalid status) - Add early return pattern to reduce nesting - Use enum comparison instead of string comparison for status check
…i#22) Implement ability to download files that require authentication: - Basic Auth (username/password) - Bearer Token (OAuth-style) - Cookie-based authentication - Custom HTTP headers Changes: - Add AuthenticationType enum with 5 auth modes - Add 6 auth properties to FileDownloadQueueItem model - Create HttpRequestAuthenticator helper for applying auth - Update DownloadQueueService to apply auth at 5 HTTP request points - Add Authentication Settings UI to DownloadFromDirectLink page - Add 19 unit tests for HttpRequestAuthenticator - Include EF Core migration for new database columns Tested with httpbin.org authentication endpoints.
…gudarzi#22)" This reverts commit 0bb3fb1.
Summary
This PR fixes 4 bugs discovered during Playwright testing, plus a test project version mismatch.
Bug #4 (Critical): Spotify Search crashes application
SpotifySearchService.cscalledEnsureSuccessStatusCode()without try-catch, causing unhandled exceptions when the API returns non-2xx statusErrorproperty toSpotifySearchResultMudAlertinSpotifySearch.razorBug #1: Incorrect file extension inference (.exe for binary data)
Helpers.csmappedapplication/octet-streamto.exe.bin- generic binary data should not default to executable extensionBug #3: "Autostart Paused Downloads" not working reliably
StartPausedDownloads()only called once during initial loadStartPausedDownloads()when downloads finish/fail and when restoring from persisted stateBug #5: Proxy Server setting persists unexpectedly
ProxyUrlhad hardcoded defaulthttp://localhost:8086Test project fix
Microsoft.AspNetCore.Identity.EntityFrameworkCorefrom 8.0.22 to 8.0.23 (version mismatch)Microsoft.EntityFrameworkCore.InMemoryPackageReferenceTest plan
.bininstead of.exeforapplication/octet-streamFiles changed
SaveHere/Services/SpotifySearchService.csSaveHere/Components/Utility/SpotifySearch.razorSaveHere/Helpers/Helpers.csSaveHere/Components/Download/DownloadVideoAudio.razorSaveHere.Tests/SaveHere.Tests.csproj