Skip to content

fix: review round 2 — per-browser UA cache, leak fixes, docs#2

Merged
desperatee merged 1 commit into
mainfrom
fix/review-round2-and-docs
Jul 1, 2026
Merged

fix: review round 2 — per-browser UA cache, leak fixes, docs#2
desperatee merged 1 commit into
mainfrom
fix/review-round2-and-docs

Conversation

@desperatee

Copy link
Copy Markdown
Contributor

Summary

  • CRITICAL fix: UA cache moved from global package variable to per-browser sync.Once — fixes incorrect cache sharing across browsers with different versions/channels, eliminates global mutex contention at scale
  • HIGH fix: Context leak in NewStealthPage when NewPage() fails
  • HIGH fix: Panic-safe type assertion in getPatchedUA
  • HIGH fix: routeInjecting flag reset on Route() failure so retries work
  • Docs: README updated with stealth API section, CLAUDE.md updated with new files, LLM guide and example updated

Review findings addressed

# Severity Issue Fix
1 CRITICAL Global patchedUACache wrong for multi-browser fleets Per-browser sync.Once on browserImpl
3 HIGH NewStealthPage leaks context on NewPage failure context.Close() on error path
4 HIGH Unchecked type assertion can panic in getPatchedUA Comma-ok form + error return
4b HIGH ctx.Close() errors silently discarded Logged via package logger
5 HIGH routeInjecting stuck on Route() failure Reset to false on error

Test plan

  • All unit tests pass
  • Integration test passes
  • E2E: Walmart homepage returns 200 with stealth page
  • go build ./... and go vet ./... clean

Review fixes:
- CRITICAL: Move UA cache from global package var to per-browser
  sync.Once on browserImpl. Fixes incorrect cache sharing across
  browsers with different versions/channels. Eliminates global mutex
  contention during 50+ browser cold starts.
- HIGH: Fix context leak in NewStealthPage when NewPage fails (add
  context.Close on error path)
- HIGH: Guard type assertion in getPatchedUA with comma-ok to prevent
  panic on unexpected navigator.userAgent type
- HIGH: Log ctx.Close errors in getPatchedUA instead of discarding
- HIGH: Reset routeInjecting atomic.Bool on Route failure in both
  page.go and browser_context.go so retries aren't silently skipped

Docs:
- README: Add stealth API section, update quick start to use
  NewStealthPage, add Chrome channel docs, update credits
- CLAUDE.md: Add stealth.go and new files to file structure, document
  patchrightInitScript and stealth API
- LLM guide: Update concurrent browsers example to use NewStealthPage
- Example: Update basic example to use NewStealthPage
@desperatee desperatee merged commit 2874ae3 into main Jul 1, 2026
4 checks passed
@desperatee desperatee deleted the fix/review-round2-and-docs branch July 1, 2026 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant