feat(wallpaper): add Wallhaven proxy route + sectioned picker integration#1902
feat(wallpaper): add Wallhaven proxy route + sectioned picker integration#1902hognek wants to merge 3 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 38 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 (1)
📝 WalkthroughWalkthroughAdds a Wallhaven API proxy with validation, error handling, and optional API-key forwarding, plus a debounced desktop search browser and wallpaper picker integration for applying selected online images. ChangesWallhaven wallpaper browsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant WallpaperPicker
participant WallhavenBrowser
participant WallhavenSearchRoute
participant WallhavenAPI
User->>WallpaperPicker: Expand online browsing
WallpaperPicker->>WallhavenBrowser: Render search panel
User->>WallhavenBrowser: Enter search query
WallhavenBrowser->>WallhavenSearchRoute: Request debounced query
WallhavenSearchRoute->>WallhavenAPI: Forward validated search
WallhavenAPI-->>WallhavenSearchRoute: Return results
WallhavenSearchRoute-->>WallhavenBrowser: Return search JSON
WallhavenBrowser-->>User: Render wallpaper thumbnails
User->>WallhavenBrowser: Select wallpaper
WallhavenBrowser->>WallpaperPicker: Pass URL and label
WallpaperPicker-->>User: Apply wallpaper to active theme
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // Apply remote wallpaper directly — set background-image to the full URL | ||
| useThemeStore.setState({ | ||
| wallpaperId: `wallhaven-${Date.now()}`, | ||
| wallpaperImage: `url('${url}')`, |
There was a problem hiding this comment.
WARNING: Unescaped remote URL interpolated into a CSS url('...') value
The selected Wallhaven URL is placed directly inside a CSS string via `url('${url}')`. If a URL ever contains a single quote (e.g. tracked query params such as ?a=b&c='d), the CSS value breaks and the wallpaper silently fails to load. More importantly, arbitrary URL content is flowing unescaped into a style value. Build the url() value with the URL escaped, e.g. `url("${url.replace(/\"/g, '\\"')}")` (or better, set backgroundImage via a JS object style rather than a string), so a stray quote cannot break rendering or be abused.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <WallhavenBrowser | ||
| onSelect={(url, label) => { | ||
| // Apply remote wallpaper directly — set background-image to the full URL | ||
| useThemeStore.setState({ |
There was a problem hiding this comment.
SUGGESTION: Applying wallpaper via useThemeStore.setState bypasses setWallpaper, leaving state inconsistent
setWallpaper (theme-store.ts:244) records the choice in wallpaperIdByTheme[activeThemeId] and also sets the light/dark/mobile variants (wallpaperLightImage, wallpaperMobileImage, etc.). Calling setState directly sets only wallpaperImage/wallpaperKind/wallpaperOverlayText and a synthetic wallpaperId. Consequences:
- The pick is never remembered in
wallpaperIdByTheme, so switching themes and back drops the selected online wallpaper. wallpaperMobileImage/wallpaperFallbackare not updated; on mobile or fallback paths the online wallpaper may not render.wallpaperId(wallhaven-<Date.now()>) is not a tracked wallpaper, soaria-pressed/aria-pressedstyling in WallpaperPicker never reflects it.
Consider routing through setWallpaper or adding a dedicated setRemoteWallpaper(url) action that fills all the same fields wallpaperFields does.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if api_key: | ||
| headers["X-API-Key"] = api_key | ||
|
|
||
| params: dict[str, str | int] = { |
There was a problem hiding this comment.
SUGGESTION: categories and purity query params are forwarded to Wallhaven with no validation
These are user-supplied strings passed straight into upstream params. Wallhaven expects a 3-char bitmask (each char 0/1). Invalid values such as categories=999, purity=abc, or an over-long string are sent upstream unvalidated. Wallhaven will reject them, but the proxy should validate the format (and length) before forwarding, returning a 400 rather than proxying a known-bad request. At minimum constrain to /^[01]{1,3}$/ or enforce exactly 3 chars.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| status_code=502, | ||
| ) | ||
|
|
||
| return JSONResponse(resp.json()) |
There was a problem hiding this comment.
WARNING: resp.json() is returned directly without error handling
If Wallhaven returns a 200 with a non-JSON or malformed body (rare, but possible on gateway/proxy errors), resp.json() raises and the route returns a 500 with an unhandled traceback instead of the friendly error envelope used elsewhere. Wrap the parse in try/except and return a 502 with a clear message, consistent with the other error branches. Also consider bounding the response size (httpx max_redirects/stream + limits) to avoid proxying an unexpectedly large payload.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| config_path=path, | ||
| ) | ||
| # Wallhaven API key is env-only (never written to config.yaml) | ||
| import os as _os |
There was a problem hiding this comment.
SUGGESTION: import os as _os is performed inside the function body
Imports are conventionally placed at module top level. Function-local imports work but are unusual here, add per-call overhead, and diverge from the rest of config.py. Move import os (or from os import environ) to the top of the file alongside the other imports.
| import os as _os | |
| cfg.wallhaven_api_key = os.environ.get("WALLHAVEN_API_KEY") or None |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Previous Findings — Resolved (incremental)
Files Reviewed (incremental, 2 files with code changes)
Fix these issues in Kilo Cloud Previous Review Summaries (4 snapshots, latest commit a682f0e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a682f0e)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Previous Findings — ResolvedAll 5 findings from the prior review (commit
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit 64f0eea)Status: No Issues Found | Recommendation: Merge All 5 findings from the previous review (commit Resolutions (previously reported, now fixed)
Files Reviewed (7 files)
Previous review (commit 1520219)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit 2584d1b)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by hy3:free · Input: 94.9K · Output: 14K · Cached: 818.2K |
|
Thanks — the proxy/security core is clean (no SSRF, auth-gated, API key env-only, 12 tests pass). Two things before merge:
Optional hardening (non-blocking): wrap |
2584d1b to
1520219
Compare
hognek
changed the title
|
Held on two real (if low-severity) correctness issues plus the Kilo findings. Security core is clean (no SSRF,
|
1520219 to
64f0eea
Compare
|
Pushed fixes for the 4 items flagged for merge:
Tests: config 30/30, frontend 22/22, wallhaven targeted 3/3 (test_search_pagination has pre-existing conftest hang — will check CI matrix). |
|
The fold looks right, thanks, but the branch is now CONFLICTING against dev (it advanced). Please rebase onto origin/dev and I'll re-review + merge. |
64f0eea to
a682f0e
Compare
| @router.get("/api/wallhaven/search") | ||
| async def wallhaven_search( | ||
| request: Request, | ||
| q: str = Query(default="", description="Search term"), |
There was a problem hiding this comment.
SUGGESTION: q (and sorting) are forwarded to Wallhaven with no validation or length cap.
q defaults to "", so a direct call to this endpoint with no q proxies to Wallhaven's unfiltered "latest uploads" feed, and sorting accepts arbitrary values. Consider making q required (or at least rejecting empty/whitespace) and adding a max length (e.g. max_length=200) to both q and sorting, matching the validation already applied to categories/purity. This keeps the public proxy contract safe from unbounded/abusive upstream queries.
| q: str = Query(default="", description="Search term"), | |
| q: str = Query(default="", min_length=1, max_length=200, description="Search term"), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/test_wallhaven.py (1)
110-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new validation and response guards.
Add cases for invalid
categories/purity, malformed JSON, and a body exceeding_MAX_RESPONSE_BYTES. These are important defensive branches introduced by this route.🤖 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 `@tests/test_wallhaven.py` around lines 110 - 186, Add tests for the Wallhaven search route covering invalid categories or purity parameters, malformed upstream JSON, and a response body larger than _MAX_RESPONSE_BYTES. Assert each case returns the expected validation or guarded error response, using the existing respx setup and client request patterns in the test suite.
🤖 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 `@desktop/src/components/WallhavenBrowser.tsx`:
- Around line 81-85: Update the error-response handling in WallhavenBrowser so
it rechecks request cancellation or staleness after awaiting resp.json() and
before calling setErrorMsg or setStatus. Return without updating state when the
request is obsolete, while preserving the existing error message and status
behavior for the active request.
- Line 43: Initialize the debounceRef useRef in WallhavenBrowser with an
explicit undefined value while retaining its ReturnType<typeof setTimeout> type,
so it satisfies React 19 typings.
- Around line 110-113: WallhavenBrowser currently passes the details-page URL
instead of the direct image asset to WallpaperPicker. In
desktop/src/components/WallhavenBrowser.tsx lines 110-113, update handleSelect
to pass img.path; in desktop/src/components/WallhavenBrowser.test.tsx lines 9-10
and 22-23, swap the fixture url/path values; at lines 166-170, retain the
expectation for the direct image URL.
In `@desktop/src/components/WallpaperPicker.tsx`:
- Around line 181-229: The WallhavenBrowser currently renders in a shrink-0
footer outside the dialog’s scrollable content, causing multi-row results and
pagination to be clipped. Move the “Browse online” toggle and WallhavenBrowser
into the mapped `online` section’s existing overflow-y-auto panel, removing the
duplicate “Search Wallhaven” content or replacing it with this browser while
preserving the existing selection state update behavior.
In `@tinyagentos/config.py`:
- Around line 181-182: The configuration loader must read WALLHAVEN_API_KEY
before the missing-file branch and pass that value into both AppConfig
construction paths; update tinyagentos/config.py lines 181-182 accordingly. In
tests/test_wallhaven.py lines 188-211, set the environment variable before
creating configuration or app state and cover both missing and existing
config-file scenarios without mutating app state to bypass loading.
In `@tinyagentos/routes/wallhaven.py`:
- Around line 55-61: Update the upstream request flow around the AsyncClient
call and response handling to use client.stream(...) with incremental
aiter_bytes() reads. Accumulate and count chunks as they arrive, abort once the
total exceeds _MAX_RESPONSE_BYTES, and preserve the existing response processing
for bodies within the limit; remove reliance on AsyncClient.get() buffering
before the size check.
---
Nitpick comments:
In `@tests/test_wallhaven.py`:
- Around line 110-186: Add tests for the Wallhaven search route covering invalid
categories or purity parameters, malformed upstream JSON, and a response body
larger than _MAX_RESPONSE_BYTES. Assert each case returns the expected
validation or guarded error response, using the existing respx setup and client
request patterns in the test suite.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72aff574-62a4-4917-b0ec-e5640380a4aa
📒 Files selected for processing (7)
desktop/src/components/WallhavenBrowser.test.tsxdesktop/src/components/WallhavenBrowser.tsxdesktop/src/components/WallpaperPicker.tsxtests/test_wallhaven.pytinyagentos/config.pytinyagentos/routes/__init__.pytinyagentos/routes/wallhaven.py
| try: | ||
| async with httpx.AsyncClient(timeout=15) as client: | ||
| resp = await client.get( | ||
| f"{WALLHAVEN_BASE}/search", | ||
| params=params, | ||
| headers=headers, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Stream the upstream body to enforce the 1 MB limit.
AsyncClient.get() buffers the complete response before Line 86 checks its size. A large response can therefore consume unbounded memory despite the configured cap. Use client.stream(...), count chunks while reading, and abort once _MAX_RESPONSE_BYTES is exceeded.
#!/bin/bash
# Confirm the declared HTTPX version and locate established streaming patterns.
fd -a -t f '^(pyproject\.toml|uv\.lock|poetry\.lock|requirements.*\.txt)$' . \
-x sh -c 'echo "=== $1"; rg -n -C2 "\bhttpx\b" "$1"' sh {}
rg -n --type=py -C3 '\.stream\s*\(|aiter_bytes\s*\(' tinyagentos testsAlso applies to: 85-90
🤖 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 `@tinyagentos/routes/wallhaven.py` around lines 55 - 61, Update the upstream
request flow around the AsyncClient call and response handling to use
client.stream(...) with incremental aiter_bytes() reads. Accumulate and count
chunks as they arrive, abort once the total exceeds _MAX_RESPONSE_BYTES, and
preserve the existing response processing for bodies within the limit; remove
reliance on AsyncClient.get() buffering before the size check.
|
Rebased onto origin/dev (e895f71). Resolved conflicts in config.py and WallpaperPicker.tsx. PR is now mergeable — config tests (35/35) pass. |
…des #1902) (#1982) * feat(wallpaper): add Wallhaven proxy route + browse-online picker section - Add wallhaven_api_key config field (env-only, never in repo) - Create GET /api/wallhaven/search proxy route to wallhaven.cc API - Keyless by default; optional X-API-Key header when WALLHAVEN_API_KEY set - Handle rate limiting (429), timeouts (504), and Wallhaven errors (502) - New WallhavenBrowser component: debounced search, thumbnail grid, pagination - Integrate WallhavenBrowser into WallpaperPicker as collapsible section - WallpaperPicker: "Browse online" toggle expands search UI, selecting a Wallhaven image applies it as a remote wallpaper - Backend tests (test_wallhaven.py, 12 tests) with respx mocking - Frontend tests (WallhavenBrowser.test.tsx, 8 tests; WallpaperPicker 14 tests) Fixes #864 * fix(wallpaper): escape CSS url, persist wallpaperIdByTheme, guard JSONResponse, validate categories/purity, move os import - Escape single-quotes and backslashes in remote wallpaper URLs to prevent CSS injection and render breakage (WallpaperPicker.tsx onSelect). - Route through a state updater that sets wallpaperIdByTheme, and include light/mobile/fallback variants so theme switch preserves remote wallpapers. - Use the received label as wallpaperOverlayText instead of discarding it. - Guard resp.json() with try/except ValueError and cap response at 1 MB (wallhaven.py:71). - Validate categories/purity with ^[01]{3}$ before forwarding to Wallhaven. - Move import os as _os from inside load_config to module top (config.py). * fix(wallpaper): bot-fix v2 — Kilo 1S + CodeRabbit 6 findings on Wallhaven - wallhaven.py: add max_length=200 on q/sorting params, validate sorting against known Wallhaven values, use client.stream() + aiter_bytes() to enforce 1MB limit incrementally (CR6) - WallhavenBrowser.tsx: init useRef with undefined (React 19, CR1), recheck cancellation after resp.json() (CR2), pass img.path not img.url to onSelect (CR3) - WallpaperPicker.tsx: move WallhavenBrowser into Online section's scrollable panel instead of shrink-0 footer (CR4) - config.py: read WALLHAVEN_API_KEY before missing-file branch so fresh installs pick it up (CR5) - tests: swap url/path fixtures to match real Wallhaven API, add sorting validation + max_length + api_key fresh-install tests * fix(wallpaper): balance JSX closers in WallpaperPicker + add CSRF dependency on wallhaven router Remove unbalanced </div> that broke the desktop build after CR4 restructure (WallhavenBrowser moved into Online section). The .map() callback and p-4 wrapper were missing closing tags; add ))} and </div> to properly close both. Also add missing dependencies=_csrf on the wallhaven router registration in routes/__init__.py for consistency with every sibling router. Docs-Reviewed: wallpaper proxy is a protected internal route, no external API docs needed * fix(wallpaper): use getByRole heading to disambiguate Browse online test query The 'renders section headings for all four sections' test used getByText('Browse online') which now matches both the section heading (h4) and the browser-toggle button inside the Online section. Scope the query to getByRole('heading', ...) so it only matches the h4. --------- Co-authored-by: Hogne <227774406+hognek@users.noreply.github.com>
…tion - Add wallhaven_api_key config field (env-only, never in repo) - Create GET /api/wallhaven/search proxy route to wallhaven.cc API - Keyless by default; optional X-API-Key header when WALLHAVEN_API_KEY set - Handle rate limiting (429), timeouts (504), and Wallhaven errors (502) - New WallhavenBrowser component: debounced search, thumbnail grid, pagination - Integrate WallhavenBrowser into WallpaperPicker as collapsible section - WallpaperPicker: "Browse online" toggle expands search UI, selecting a Wallhaven image applies it as a remote wallpaper - Backend tests (test_wallhaven.py, 12 tests) with respx mocking - Frontend tests (WallhavenBrowser.test.tsx, 8 tests; WallpaperPicker 14 tests) Fixes jaylfc#864
a682f0e to
d1d7fb4
Compare
…NResponse, validate categories/purity, move os import
- Escape single-quotes and backslashes in remote wallpaper URLs to prevent
CSS injection and render breakage (WallpaperPicker.tsx onSelect).
- Route through a state updater that sets wallpaperIdByTheme, and include
light/mobile/fallback variants so theme switch preserves remote wallpapers.
- Use the received label as wallpaperOverlayText instead of discarding it.
- Guard resp.json() with try/except ValueError and cap response at 1 MB
(wallhaven.py:71).
- Validate categories/purity with ^[01]{3}$ before forwarding to Wallhaven.
- Move import os as _os from inside load_config to module top (config.py).
d1d7fb4 to
8f8feb4
Compare
| onSelect={(url, label) => { | ||
| // Escape single-quotes and backslashes to prevent CSS injection | ||
| // and render breakage from special characters in remote URLs. | ||
| const safeUrl = url.replace(/\\/g, "\\\\").replace(/'/g, "\\'"); |
There was a problem hiding this comment.
WARNING: CSS url() escaping is incomplete — still allows injection / render breakage
Only backslashes and single-quotes are escaped. A remote URL containing a literal ) (valid in many query strings, e.g. ?a=b)) prematurely terminates url('...'), and a value like x)'); color:red; // becomes url('x)'); color:red; //') — a real CSS-injection / style-breakage vector. Control characters (\n, \r, \0) are also unescaped. Escape/strip ), (, and whitespace/control chars, and ideally validate the scheme is http(s) before building url('...').
| const safeUrl = url.replace(/\\/g, "\\\\").replace(/'/g, "\\'"); | |
| const safeUrl = url.replace(/[\\()'\r\n\0]/g, (c) => `\\${c}`); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…rol chars, validate http(s) scheme
Kilo WARNING: CSS url() escaping was incomplete. Only backslashes and
single-quotes were escaped, allowing ')' in query strings to prematurely
terminate url('...') and inject CSS. Now also:
- Escape '(' as %28 and ')' as %29
- Strip control characters (\x00-\x1f, \x7f)
- Validate scheme is http(s) before proceeding
Also removes a duplicate 'Browse online' section left behind during
the sectioned-picker rebase — changes are low-risk and build/vitest pass.
|
I have read the CLA Document and I hereby sign the CLA |
2 participants
Changes
Fixes #864
Summary by CodeRabbit