feat: support multi-server fallback in the userscript - #106
Conversation
Allow configuring several Web UI addresses and probe them in order before opening a download, and only update feed position checkpoints while that modal is open.
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe userscript now supports ordered, deduplicated Web UI server addresses with reachability fallback. Download and queue flows resolve servers asynchronously. Feed checkpoint and playback recording require an open feed navigator. ChangesUserscript behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant openDownload
participant resolveWebuiBase
participant WebUI
User->>openDownload: request download
openDownload->>resolveWebuiBase: resolve configured server
resolveWebuiBase->>WebUI: probe servers in order
WebUI-->>resolveWebuiBase: return reachable response or failure
resolveWebuiBase-->>openDownload: return resolved base or error
openDownload-->>User: open panel/tab or show error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
userscript/sc-gate-dl.user.js (1)
2481-2501: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winQueued downloads are lost when no configured server is reachable.
openDownloadbecame asynchronous and can fail afterresolveWebuiBaserejects, but it returnsundefinedin every path. Both queue callers remove the item fromdownloadQueuebefore they invoke it, and neither awaits the result, so a resolution failure drops the item with only an alert.
userscript/sc-gate-dl.user.js#L2481-L2501: returnfalseon the resolution failure path andtrueon each success path, so callers can detect failure. Also move theisPanelBusy()check so the panel state is not read across the await window.userscript/sc-gate-dl.user.js#L2393-L2402: await or chain theopenDownloadresult. If it returnsfalse,unshiftthe item back ontodownloadQueueand callrenderQueue().userscript/sc-gate-dl.user.js#L2404-L2417: await or chain theopenDownloadresult. If it returnsfalse, re-insert the spliced item at its original index and callrenderQueue().🤖 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 `@userscript/sc-gate-dl.user.js` around lines 2481 - 2501, Update userscript/sc-gate-dl.user.js:2481-2501 in openDownload to check isPanelBusy() before awaiting resolveWebuiBase, return false when resolution fails, and return true from every successful path. At userscript/sc-gate-dl.user.js:2393-2402, await or chain openDownload and unshift the item back into downloadQueue followed by renderQueue() when it returns false. At userscript/sc-gate-dl.user.js:2404-2417, similarly handle false by reinserting the spliced item at its original index and calling renderQueue().
🧹 Nitpick comments (2)
userscript/sc-gate-dl.user.js (2)
1231-1237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd Escape-key dismissal to the server dialog.
The dialog sets
role="dialog"and receives focus on the textarea. A keyboard user can only close it with the Cancel button, because click-outside is the only other exit. The panel at Line 2639 already handles Escape. Add the same handling here for consistency.♻️ Proposed addition
const close = () => dialog.remove(); + const onKeydown = (event) => { + if (event.key === 'Escape') close(); + }; + dialog.addEventListener('keydown', onKeydown); dialog.addEventListener('click', (event) => { if (event.target === dialog) close(); });🤖 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 `@userscript/sc-gate-dl.user.js` around lines 1231 - 1237, Update the server dialog setup around the close function and existing click handlers to listen for keydown events and call close when the pressed key is Escape, matching the Escape-dismissal behavior used by the panel at line 2639.
1074-1090: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAlign read precedence with write precedence.
persistWebuiBaseswritesGM_setValuefirst, thenlocalStorage.readStoredWebuiRawreadslocalStoragefirst, thenGM_getValue. If thelocalStoragewrite fails while theGM_setValuewrite succeeds, the stalelocalStoragevalue wins on the next read, and the saved server list is silently ignored.getApiBase(Line 2233) already readsGM_getValuefirst.Prefer the GM store on read for consistency.
♻️ Proposed read-order change
function readStoredWebuiRaw() { - try { - const stored = localStorage.getItem(WEBUI_BASE_KEY); - if (stored?.trim()) return stored; - } catch { - // ignore - } try { if (typeof GM_getValue === 'function') { const stored = GM_getValue(WEBUI_BASE_KEY, null); if (typeof stored === 'string' && stored.trim()) return stored; } } catch { // ignore } + try { + const stored = localStorage.getItem(WEBUI_BASE_KEY); + if (stored?.trim()) return stored; + } catch { + // ignore + } return null; }🤖 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 `@userscript/sc-gate-dl.user.js` around lines 1074 - 1090, Update readStoredWebuiRaw to check GM_getValue before localStorage, matching persistWebuiBases and getApiBase precedence. Preserve the existing validation, fallback behavior, and error handling while returning the first non-empty stored value.
🤖 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 `@userscript/sc-gate-dl.user.js`:
- Around line 1171-1209: Reduce download-path probe latency in resolveWebuiBase
by caching a recently successful activeWebuiBase for a short TTL and trying it
first before probing other bases. Add transient user-visible status around the
await in openDownload so probing does not appear stalled. Keep
isWebuiReachable’s API validation authoritative where possible, and do not treat
a no-cors response alone as proof that the responder is a sc-gate-dl Web UI.
- Around line 1240-1271: Update the save click handler to await the asynchronous
resolveWebuiBase/loadTrackIntoPanel flow before displaying the success
window.alert, and only report the configured servers after resolution succeeds.
Preserve the existing failure alert for rejected resolution and avoid showing
the success alert when resolution fails.
- Line 1178: Update the request configuration using AbortSignal.timeout to
support browsers lacking this API by adding a compatible timeout fallback, or
document and enforce the minimum required browser versions. Preserve the
existing WEBUI_REACHABILITY_TIMEOUT_MS timeout behavior.
---
Outside diff comments:
In `@userscript/sc-gate-dl.user.js`:
- Around line 2481-2501: Update userscript/sc-gate-dl.user.js:2481-2501 in
openDownload to check isPanelBusy() before awaiting resolveWebuiBase, return
false when resolution fails, and return true from every successful path. At
userscript/sc-gate-dl.user.js:2393-2402, await or chain openDownload and unshift
the item back into downloadQueue followed by renderQueue() when it returns
false. At userscript/sc-gate-dl.user.js:2404-2417, similarly handle false by
reinserting the spliced item at its original index and calling renderQueue().
---
Nitpick comments:
In `@userscript/sc-gate-dl.user.js`:
- Around line 1231-1237: Update the server dialog setup around the close
function and existing click handlers to listen for keydown events and call close
when the pressed key is Escape, matching the Escape-dismissal behavior used by
the panel at line 2639.
- Around line 1074-1090: Update readStoredWebuiRaw to check GM_getValue before
localStorage, matching persistWebuiBases and getApiBase precedence. Preserve the
existing validation, fallback behavior, and error handling while returning the
first non-empty stored value.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3134ac52-d9a7-4ae1-b875-7c6f0fc4c62b
📒 Files selected for processing (3)
README.mduserscript/sc-gate-dl.test.tsuserscript/sc-gate-dl.user.js
Cache the last reachable server briefly, polyfill AbortSignal.timeout, avoid stacked configure alerts, and put failed queue items back.
|
Composer responding on behalf of Nico Also addressed the outside-diff queue note from CodeRabbit in 3ce15bd: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
userscript/sc-gate-dl.user.js (1)
2518-2531: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve user activation before server resolution.
Line 2520 awaits network I/O before Line 2530 calls
window.open(). Browsers can clear transient user activation during this wait. ThegetAlwaysOpenTab()flow can then fail to open a tab after a user click.Open a placeholder tab synchronously before the first
await, then navigate it after resolution succeeds. If the browser denies the placeholder tab, returnfalseand show a failure message. This also lets Lines 2428-2432 and Lines 2449-2453 restore queued items when an automatic tab open is blocked.🤖 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 `@userscript/sc-gate-dl.user.js` around lines 2518 - 2531, Update openDownload so the getAlwaysOpenTab flow opens a placeholder tab synchronously before the first await, retaining its window reference. If the placeholder is blocked, show a failure alert and return false; after resolveWebuiBase succeeds, navigate the placeholder to buildWebuiSrc(trackUrl), and close it on resolution failure before returning false. Preserve the existing non-tab flow and return behavior.
🤖 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.
Outside diff comments:
In `@userscript/sc-gate-dl.user.js`:
- Around line 2518-2531: Update openDownload so the getAlwaysOpenTab flow opens
a placeholder tab synchronously before the first await, retaining its window
reference. If the placeholder is blocked, show a failure alert and return false;
after resolveWebuiBase succeeds, navigate the placeholder to
buildWebuiSrc(trackUrl), and close it on resolution failure before returning
false. Preserve the existing non-tab flow and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 471ab766-b9e5-4a53-9eab-e88fc1c1f418
📒 Files selected for processing (2)
userscript/sc-gate-dl.test.tsuserscript/sc-gate-dl.user.js
🚧 Files skipped from review as they are similar to previous changes (1)
- userscript/sc-gate-dl.test.ts
Open a placeholder tab before probing servers so browsers do not drop user activation during the await.
|
Composer responding on behalf of Nico Addressed the follow-up always-open-tab note in the latest commit: on a cache miss we open |
1 participant
Summary
Feed position checkpoints were always updating in the background, and the userscript could only point at a single Web UI address — awkward when switching between LAN and Tailscale.
Test plan
http://localhost:4321bun test userscript/sc-gate-dl.test.tsMade with Cursor (Composer)
Summary by CodeRabbit