Fix search hydration crashes on bad HTML and empty domains - #199
Closed
Alien10140 wants to merge 3 commits into
Closed
Fix search hydration crashes on bad HTML and empty domains#199Alien10140 wants to merge 3 commits into
Alien10140 wants to merge 3 commits into
Conversation
Search jobs could die mid-run when a page returned non-UTF-8 HTML (MySQL utf8mb4 reject) or when error logging tried to cache Closure stack frames. Empty/null result domains also typed-errored domainFilter. Keep the job alive: sanitize HTML on persist, log failures without serializable traces, and harden domain matching.
Contributor
📝 WalkthroughWalkthroughSearch execution now supports quiet logging and explicit result filtering. Domain handling tolerates null or blank values, URL store lookup validates hosts, and hydrated research persistence sanitizes HTML and isolates failures. Tests cover these behaviors. ChangesSearch and domain persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app/Services/SearchService.php`:
- Around line 515-518: Update replaceLastLogEntry() in
app/Services/SearchService.php at lines 515-518 to return immediately when quiet
mode is enabled, before popping or writing the cache. Add coverage in
tests/Feature/Services/SearchServiceTest.php at lines 270-276 by seeding a log
entry, invoking replaceLastLogEntry() in quiet mode, and asserting the entry
remains unchanged.
- Around line 348-360: Update both per-result recovery catch blocks in
SearchService: the hydration handler at app/Services/SearchService.php lines
348-360 and the sanitization/persistence handler at lines 404-420. Catch
Throwable instead of Exception in each block so TypeError and other
non-exception errors are handled by the existing recovery logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad8f6b90-482f-44c9-ba02-f090af555536
📒 Files selected for processing (6)
app/Models/Store.phpapp/Models/UrlResearch.phpapp/Services/ScrapeUrl.phpapp/Services/SearchService.phptests/Feature/Models/StoreTest.phptests/Feature/Services/SearchServiceTest.php
Catch Throwable in per-result hydration/persist so TypeError cannot abort the job. Make replaceLastLogEntry honor quiet mode before mutating the log cache. Prefer UTF-8 repair before Windows-1252 conversion.
Avoid re-decoding mostly-UTF-8 pages as Windows-1252 when invalid bytes are present alongside real multi-byte sequences.
Contributor
Author
|
Addressed CodeRabbit feedback:
|
Contributor
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)
app/Services/SearchService.php (1)
405-415: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid caching failed hydrations.
The call at Line 368 persists the result even when hydration failed. The supplied failure test confirms this creates a
UrlResearchrow; on the next run, Lines 328-335 treat that row as a valid cache hit and skip hydration, turning a transient scrape failure into a stale permanent miss.Track hydration success and skip persistence for failed new hydrations, or mark failed rows so
getUrlResearch()excludes them. Add a two-run regression test.🤖 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 `@app/Services/SearchService.php` around lines 405 - 415, Update the hydration flow in SearchService around getUrlResearch() and the UrlResearch::updateOrCreate() call to distinguish successful hydrations from failures, and skip caching newly hydrated results when hydration fails. Preserve valid existing cache behavior, and add a regression test covering two runs: the first failed hydration must create no valid cache hit, allowing the second run to retry hydration.
🧹 Nitpick comments (1)
app/Services/SearchService.php (1)
427-450: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd mixed-encoding regression coverage.
The existing test covers only a single Windows-1252 byte. Add a case containing valid UTF-8 bytes (for example
é) alongside an invalid legacy byte, asserting the UTF-8 text is preserved while the invalid byte is removed. This directly protects the new branch at Lines 441-448.🤖 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 `@app/Services/SearchService.php` around lines 427 - 450, Extend the tests for SearchService::sanitizeUtf8 with a mixed-encoding input containing valid UTF-8 text such as é plus an invalid legacy byte. Assert the result preserves the valid UTF-8 text while removing the invalid byte, covering the repair branch without changing the existing single-byte Windows-1252 case.
🤖 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 `@app/Services/SearchService.php`:
- Around line 405-415: Update the hydration flow in SearchService around
getUrlResearch() and the UrlResearch::updateOrCreate() call to distinguish
successful hydrations from failures, and skip caching newly hydrated results
when hydration fails. Preserve valid existing cache behavior, and add a
regression test covering two runs: the first failed hydration must create no
valid cache hit, allowing the second run to retry hydration.
---
Nitpick comments:
In `@app/Services/SearchService.php`:
- Around line 427-450: Extend the tests for SearchService::sanitizeUtf8 with a
mixed-encoding input containing valid UTF-8 text such as é plus an invalid
legacy byte. Assert the result preserves the valid UTF-8 text while removing the
invalid byte, covering the repair branch without changing the existing
single-byte Windows-1252 case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 43fc2868-6905-471f-bb7c-d10d9507e935
📒 Files selected for processing (2)
app/Services/SearchService.phptests/Feature/Services/SearchServiceTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Feature/Services/SearchServiceTest.php
This was referenced Aug 7, 2026
Merged
Owner
|
thanks, merged here #214 as it needed a bunch of fixes |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
url_researchStore::domainFilter/ empty domains so SearchXNG hits without a host cannot TypeError the jobWhy
A single legacy page (invalid UTF-8) could:
utf8mb4persistence, then$e->getTrace()(Closures are not serializable),leaving the UI stuck on “Analyzing …” forever because
completewas never set.Empty/null domains from some search hits caused a separate hard crash in
scopeDomainFilter.Related
Companion package PR (recommended together): jez500/Web-scraper-for-Laravel — sanitize UTF-8 at scrape time.
Test plan
Made with Cursor
Summary by CodeRabbit
nulland empty/invalid domain inputs, returning no matches instead of erroring.