Skip to content

fix(api): Remove throw when directOnly set to true#568

Open
DanTheMan827 wants to merge 1 commit into
monochrome-music:mainfrom
DanTheMan827:hifi-fallback
Open

fix(api): Remove throw when directOnly set to true#568
DanTheMan827 wants to merge 1 commit into
monochrome-music:mainfrom
DanTheMan827:hifi-fallback

Conversation

@DanTheMan827
Copy link
Copy Markdown

@DanTheMan827 DanTheMan827 commented Apr 17, 2026

Description

Type of Change

  • Bug fix
  • New feature
  • Style/UI update
  • Docs only

Checklist

  • I have read the Contributing Guidelines.
  • I understand every line of code I am submitting.
  • I have tested these changes locally, and they work as expected.
  • Is this Pull request Using AI/Is Vibecoded?

By submitting this PR, I agree to follow the guidelines. I understand that the final decision to merge rests with the maintainers and that not all contributions can be accepted.

Summary by CodeRabbit

  • Bug Fixes
    • Improved search reliability: fallback and retry behavior now consistently apply to both direct and unified search requests, reducing failed queries and improving result availability.

Copilot AI review requested due to automatic review settings April 17, 2026 15:04
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 48c8ae6d-6702-4760-a9cf-a2e5ad219732

📥 Commits

Reviewing files that changed from the base of the PR and between d570578 and 4777492.

📒 Files selected for processing (1)
  • js/api.js
💤 Files with no reviewable changes (1)
  • js/api.js

📝 Walkthrough

Walkthrough

Removed a conditional guard in LosslessAPI.fetchWithRetry so direct-HiFi errors no longer re-throw; failures now always continue into the instance-based fallback/retry flow. Also removed the directOnly: true override from the combined /search/?q= request in LosslessAPI.search().

Changes

Cohort / File(s) Summary
API Fallback Logic
js/api.js
Removed directOnly guard in LosslessAPI.fetchWithRetry so direct-HiFi errors proceed to instance fallback; removed forced directOnly: true from LosslessAPI.search() combined search request to allow unified retry/fallback behavior.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant LosslessAPI
    participant HiFiDirect
    participant InstanceBackend

    Client->>LosslessAPI: request (search / fetch)
    LosslessAPI->>HiFiDirect: attempt direct fetch
    HiFiDirect-->>LosslessAPI: error
    Note right of LosslessAPI: log warning and continue
    LosslessAPI->>InstanceBackend: instance-based fallback fetch/retry
    InstanceBackend-->>LosslessAPI: response
    LosslessAPI-->>Client: final response
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐰 I nibbled guards, let fallbacks play,
Direct errors now hop another way,
Logs whisper softly, retries begin,
Instances dance, the fetches win—
Rabbit cheers for smoother flow today!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description is largely incomplete. The 'Description' section is entirely empty, providing no context or rationale for the bug fix beyond the title. Add a detailed description explaining why removing the throw statement is necessary, what problem it solves, and how the fallback behavior works.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing a throw statement when directOnly is set to true, which aligns with the code modifications shown in the summary.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts LosslessAPI.fetchWithRetry / search behavior so direct-query failures no longer immediately throw when directOnly is used, allowing fallback behavior to proceed.

Changes:

  • Removed the options.directOnly early rethrow in fetchWithRetry’s direct-query path.
  • Stopped passing directOnly: true from search() when calling the combined /search/?q= endpoint.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread js/api.js

try {
// Keep direct TIDAL combined search behavior for normal mode.
// If direct query fails, fall back to hifi-api-compatible scoped searches (?s, ?a, ?al, ?v, ?p).
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says “If direct query fails, fall back to ... scoped searches”, but fetchWithRetry will now fall back to configured API instances first when the direct query fails, meaning scoped-search fallback won’t necessarily happen. Please update the comment to match the actual behavior (or adjust the logic if the intent is to always skip instance fallback here).

Suggested change
// If direct query fails, fall back to hifi-api-compatible scoped searches (?s, ?a, ?al, ?v, ?p).
// If the direct query fails, fetchWithRetry may first retry via configured API instances;
// if no usable combined result is returned, fall back to hifi-api-compatible scoped searches (?s, ?a, ?al, ?v, ?p).

Copilot uses AI. Check for mistakes.
Comment thread js/api.js
Comment on lines 80 to 81
return await HiFiClient.instance.query(relativePath);
} catch (err) {
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetchWithRetry ignores options.signal for the direct-path request (HiFiClient.instance.query(...)), so callers providing an AbortSignal/timeout won't be able to cancel direct queries. Also, if the direct request is aborted (AbortError), the current catch will swallow it and proceed to fall back to instance fetches, defeating cancellation. Pass options.signal into query(...) and rethrow AbortError in the catch (similar to the later fetch loop).

Suggested change
return await HiFiClient.instance.query(relativePath);
} catch (err) {
return await HiFiClient.instance.query(relativePath, { signal: options.signal });
} catch (err) {
if (err?.name === 'AbortError') {
throw err;
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants