Library UI: settings pane (preferred quality + per-source rules) - #2276
Library UI: settings pane (preferred quality + per-source rules)#2276jaylfc wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 54 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
PR Summary by QodoLibrary UI: add Settings view for preferred quality, storage cap, and per-source rules
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Invalid cap breaks storage
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const data = deriveMockStorageData(items, settings.storage_cap_bytes); | ||
| const totalPct = Math.min(100, (data.total_bytes / data.cap_bytes) * 100); |
There was a problem hiding this comment.
2. Invalid cap breaks storage 🐞 Bug ≡ Correctness
The settings UI can persist a storage cap of 0 (or negative), and the storage accounting view then divides by that cap and treats the cap as reached, resulting in incorrect 100% usage/progressbar values and a misleading "Paused at cap" state. This is caused by saving parseInt(...) || 0 and then using settings.storage_cap_bytes directly in deriveMockStorageData and percentage math.
Agent Prompt
### Issue description
The storage cap can be saved as `0` (or negative) and is then used as `cap_bytes` in the storage accounting calculations. This leads to incorrect percentages (division by 0) and `paused_at_cap` being effectively always true when cap is 0.
### Issue Context
- The settings input writes `storage_cap_bytes: parseInt(e.target.value) || 0`.
- The storage view uses `settings.storage_cap_bytes` as the cap and computes `totalPct` from it.
### Fix Focus Areas
- desktop/src/apps/LibraryApp.tsx[1225-1234]
- desktop/src/apps/LibraryApp.tsx[1087-1104]
- desktop/src/apps/LibraryApp.tsx[188-210]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| preferred_quality: parsed.preferred_quality ?? DEFAULT_LIBRARY_SETTINGS.preferred_quality, | ||
| storage_cap_bytes: parsed.storage_cap_bytes ?? DEFAULT_LIBRARY_SETTINGS.storage_cap_bytes, | ||
| source_rules: Array.isArray(parsed.source_rules) ? parsed.source_rules : DEFAULT_LIBRARY_SETTINGS.source_rules, | ||
| }; |
There was a problem hiding this comment.
3. Unvalidated source_rules can crash 🐞 Bug ☼ Reliability
loadLibrarySettings() accepts any array for source_rules without validating each element, but the settings table render dereferences rule.source/action/quality for every entry. If localStorage contains nullish entries (or otherwise malformed items), the settings view can throw at render time.
Agent Prompt
### Issue description
`loadLibrarySettings()` only checks `Array.isArray(parsed.source_rules)` and then returns the array verbatim. The UI later assumes each element is an object with `source`, `action`, and `quality` strings.
### Issue Context
Corrupted or legacy localStorage values (e.g., `[null]`) will cause `rule.source` dereferences to throw during rendering.
### Fix Focus Areas
- desktop/src/apps/LibraryApp.tsx[143-152]
- desktop/src/apps/LibraryApp.tsx[1256-1268]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-super review VERDICT: Found issues requiring fixes
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
HELD, not armed - this builds the CLIENT half of a feature whose SERVER half is in an open PR, and as written the two never meet. WHAT THIS PR DOES: the settings pane persists preferred quality and per-source rules to localStorage only (LIBRARY_SETTINGS_KEY). There are no API calls in the diff, and dev's tinyagentos/routes/library.py has no settings or rules endpoints at all - I grepped. WHAT ALREADY EXISTS ELSEWHERE: hognek's OPEN #2070 'feat(library): P3 - heavy tier download, quality preferences, storage accounting, per-source rules' adds precisely the server side, including POST /api/library/rules and GET /api/library/rules, with rules carrying source_pattern, quality and auto_download - the same three concepts this pane edits. CONSEQUENCE IF MERGED AS-IS: the pane would look complete and change nothing. The library pipeline downloads server-side, so rules kept in one browser's localStorage are never consulted; a user would set 720p for youtube, see it saved, and get whatever the server defaults to. That is the failure mode where the working half hides the broken half, and it is also the collision the contributing guide warns about - two PRs implementing one feature from opposite ends without knowing about each other. NOT A CRITICISM OF THE BUILD: the pane itself is sound - 194 lines of real tests, no CSRF exposure precisely because it makes no network calls, and the only CI red besides Kilo's known rate-limit fake-red is the missing changelog. TO LAND IT: wire the pane to #2070's endpoints - GET /api/library/rules to populate, POST /api/library/rules to save - and keep localStorage at most as an optimistic cache, never as the source of truth. That means this PR should either wait for #2070 to merge, or be rebased onto it. I am holding rather than closing because the UI work is worth keeping. Sequencing is mine to coordinate with hognek. |
1 participant
CARD TITLE (intent, not commit subject): Library UI: settings pane (preferred quality + per-source rules)
Autonomous build of board card tsk-g3olu7.
Files:
CHANGELOG.md | 4 -
desktop/src/apps/LibraryApp.settings.test.tsx | 194 ++++++++++++++++++++
desktop/src/apps/LibraryApp.tsx | 247 +++++++++++++++++++++++++-
docs/agent-manual/11-files-api.md | 35 ----
docs/agent-manual/index.md | 1 -
docs/taos-agent-manual.md | 35 ----
tests/test_agent_manual_compiled.py | 2 +-
7 files changed, 436 insertions(+), 82 deletions(-)