fix(wallpaper): bot-fix v2 — Kilo 1S + CodeRabbit 6 findings (supersedes #1902)#1982
Conversation
…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
…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).
…aven - 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
|
Warning Review limit reached
Next review available in: 14 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 (9)
✨ 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 |
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? |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }, | ||
| })); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
CRITICAL: JSX tags are unbalanced — this file will not compile.
<WallhavenBrowser /> is self-closed here, but its wrapper <div className="pt-2"> (line 198), the {browseOnlineOpen && ( group (line 197), and the {section.id === "online" && ( group (line 181) are never closed. Lines 227–231 emit more closing tags (</div>, )}, </div>, </div>, ))}) than were opened inside this section, so the pt-2 div and both && groups are left unterminated. This is a fatal TSX parse error (Expected corresponding JSX closing tag) that breaks the entire WallpaperPicker module and the desktop build.
Before the section's closing </div>/))}, the inner block must close in order: </div> (for pt-2), ) (for browseOnlineOpen &&), ) (for section.id === "online" &&).
| app.include_router(receipts_router, dependencies=_csrf) | ||
|
|
||
| from tinyagentos.routes import wallhaven as wallhaven_routes | ||
| app.include_router(wallhaven_routes.router) |
There was a problem hiding this comment.
SUGGESTION: Wallhaven router is registered without the _csrf dependency that every other router in this function uses (e.g. lines 389, 392, 395, 401).
This endpoint is GET-only and acts as a read proxy, so CSRF risk is low, but the inconsistency means the route is unprotected while sibling routes require CSRF. If CSRF protection is meant to be global, include dependencies=_csrf here too for consistency (or document why this route is exempt).
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Incremental Review (since 09d80ff)The only change since the previous review is a test-only edit in Previous findings (WallpaperPicker.tsx:226 JSX balance, init.py Wallhaven router registration) remain resolved at HEAD (77e8f4c). Previous Review Summaries (3 snapshots, latest commit 09d80ff)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 09d80ff)Status: No Issues Found | Recommendation: Merge Both prior findings are resolved by the incremental diff (squash at
The current HEAD ( Files Reviewed (8 files)
Previous review (commit 74edc84)Status: No Issues Found | Recommendation: Merge Both prior findings from the previous review are resolved by the incremental commits in this run:
The incremental diff ( Files Reviewed (2 files, incremental)
Previous review (commit 7a5cb34)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
SUGGESTION
Files Reviewed (8 files)
Reviewed by hy3:free · Input: 32.5K · Output: 1.4K · Cached: 104.1K |
|
Held on a build-breaker. The two correctness bugs from #1902 are genuinely fixed (CSS url() escape at WallpaperPicker.tsx:205, and the wallhaven route now streams with a 1MB cap and wraps json.loads in try/except returning 502), and CR1/CR2/CR3/CR5/CR6 are folded, thanks. But before this can merge:
Heads up on why CI did not catch the build break: this PR's checks are only Gitar/Kilo/cla/CodeRabbit - no desktop typecheck/build/vitest job ran, so green here is not evidence the desktop compiles. Fix the JSX + run the build locally and I will re-review. |
…endency 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
74edc84 to
09d80ff
Compare
…est 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.
2 participants
Task: t_cc32e61b. Supersedes #1902.
Changes
All 7 bot findings in one commit (7a5cb34):
Kilo SUGGESTION
max_length=200onqandsortingparams, validate sorting against known Wallhaven valuesCodeRabbit MAJOR/MINOR
useRef<ReturnType<typeof setTimeout>>(undefined)fixes React 19 typingscancelledafterresp.json()to prevent stale error overwriting active stateimg.path(direct image) notimg.url(details page) to onSelect. Fixtures swapped in both backend test and desktop testoverflow-y-autopanel instead ofshrink-0footerWALLHAVEN_API_KEYbefore missing-file branch so fresh installs pick it up. Both config paths now receive the keyclient.get()+resp.contentwithclient.stream()+aiter_bytes()to enforce 1MB limit incrementallyTest additions
test_config.py: 2 new tests for WALLHAVEN_API_KEY on fresh install + existing configtest_wallhaven.py: 4 new tests (invalid sorting, valid sorting forwarded, query max_length, empty query allowed)WallhavenBrowser.test.tsx: url/path fixture swapTest results
test_search_returns_resultspasses (async fixture teardown hang is pre-existing in conftest, observed on unmodified code too)