refactor(search_many): drop needless Arc<Mutex>; clarify search()/max_places docs (Closes #40, #42, #39)

[Liohtml]

Overview

Three related findings in the search path, all in src/lib.rs.

#	Change
#40	Drop needless Arc<Mutex<…>>. search_many_on_page runs sequentially on one task (no tokio::spawn shares out/seen_keys), so the Arc<Mutex<Vec>> / Arc<Mutex<HashSet>> only added uncontested async-lock overhead and a never-taken Arc::try_unwrap panic path. Replaced with plain Vec / HashSet, returning Ok(out) directly; dropped the now-unused Arc / tokio::sync::Mutex imports.
#42	search() perf doc. Documents that each call opens/closes a Chrome tab + homepage consent, so search_many should be preferred for multiple queries.
#39	max_places doc. Documents that None is truly unbounded (one navigation per place, no time bound) so callers can bound run time deliberately.

Why #39 is a doc change, not a URL truncation

The issue suggested truncating the collected URL slice to max_places before the enrich loop. That would regress results: dedup happens after navigation (the website domain is only known once the panel loads), so truncating to m URLs can yield fewer than m unique places. The existing mid-loop unique-count cap already bounds navigations when max_places is set; the genuine gap was that None is silently unbounded, which is now documented.

Test plan

• cargo build
• cargo test (9 passed)
• cargo fmt --all -- --check
• cargo clippy --all-targets -- -D warnings

Behaviour is unchanged (pure refactor + docs); the dedup/max_places/jitter logic is identical.

Closes #40, #42, #39

https://claude.ai/code/session_01TPpTHPokxsZ3dQpRzg4NkD

---

Generated by Claude Code

Summary by CodeRabbit

• Documentation

  • Clarified that single search calls open and close a tab each time, and that batch searches are the better choice for multiple queries.
  • Clarified that max_places: None returns unlimited results.

• Refactor

  • Improved the search flow to reduce unnecessary overhead during result collection and duplicate handling.

• Chores

  • Updated release notes and project metadata.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 658d06c2-5bae-47f6-8103-32e144de8b23

📥 Commits

Reviewing files that changed from the base of the PR and between ef26900 and 9fae490.

📒 Files selected for processing (2)

• CHANGELOG.md
• src/lib.rs

---

📝 Walkthrough

Walkthrough

search_many_on_page in src/lib.rs replaces Arc<Mutex<Vec<Place>>> and Arc<Mutex<HashSet<String>>> with plain owned Vec<Place> and HashSet<String>, removing all async lock/unlock calls and the Arc::try_unwrap on return. Documentation is added for ScraperConfig::max_places semantics and MapsScraper::search tab lifecycle. CHANGELOG.md records these changes.

Changes

Arc/Mutex removal and documentation updates

Layer / File(s)	Summary
Replace Arc/Mutex with owned collections in search_many_on_page src/lib.rs	Removes Arc and tokio::sync::Mutex imports. Replaces the two Arc<Mutex<…>> shared containers with mut out: Vec<Place> and mut seen_keys: HashSet<String>. All .lock().await call sites are replaced with direct mutation: seen_keys.insert/contains and out.push. The final Arc::try_unwrap(out)…into_inner() is replaced with Ok(out).
Documentation and CHANGELOG src/lib.rs, CHANGELOG.md	ScraperConfig::max_places docs describe set vs. unset behavior, deduplication counting, and enrich-mode extra-navigation caveat. MapsScraper::search gains a # Performance section noting per-call tab open/close cost and recommending search_many. CHANGELOG.md records the Arc<Mutex<…>> removal and both doc additions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• #40 — This PR directly implements the exact fix described in that issue: replacing Arc<Mutex<Vec<Place>>> and Arc<Mutex<HashSet<String>>> with plain owned containers and removing all spurious .lock().await calls and the Arc::try_unwrap dead-code path.
• #39 — Changes to search_many_on_page and max_places documentation in this PR overlap with the behavior/docs concerns tracked in that issue.
• #42 — This PR adds documentation to MapsScraper::search describing that each call opens and closes a tab and recommending search_many for multiple queries, which matches the objective of that issue.

Poem

🐇 No more locks, no Mutex maze,
A Vec and HashSet earn their praise.
Sequential code needs no shared Arc,
Direct mutation leaves its mark.
One Ok(out) to rule them all —
The rabbit hops, no mutex wall! 🎉

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/issue-39-40-42-search-cleanup

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Liohtml]

@coderabbitai review

---

Generated by Claude Code

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== PR Details ==="
gh pr view 47 --repo Liohtml/google-maps-scraper-rs

echo ""
echo "=== PR Diff ==="
gh pr diff 47 --repo Liohtml/google-maps-scraper-rs

Length of output: 9491

---

---

@Liohtml: I've reviewed the changes. The PR is clean and correct. Here's the breakdown:

---

Review of PR #47

✅ Arc<Mutex> Removal (closes #40)

The simplification is correct. search_many_on_page runs a plain sequential for loop — there is no tokio::spawn that could share out or seen_keys with another task, so Arc<Mutex<…>> was genuinely just overhead. All .lock().await call sites were correctly unwrapped to direct access, and the now-unreachable Arc::try_unwrap error path is gone. Imports cleaned up properly.

✅ search() Performance Doc (closes #42)

The new # Performance section accurately documents the per-call cost (fresh tab + consent + ~3 s settle delay) and points users toward search_many. The hardcoded sleep value mentioned in the doc (~3 s) matches tokio::time::sleep(Duration::from_secs(3)).await in the code (Line 309).

✅ max_places Doc (closes #39)

The expanded doc comment accurately captures the unbounded-None risk and the dedup nuance (enrich vs. non-enrich) so callers can make informed decisions about bounding run time.

✅ CHANGELOG

The "Changed" entries are correctly placed in the unreleased section and clearly summarise all three changes.

---

⚠️ Pre-existing issue (out of scope for this PR)

In the non-enrich path, max_places is enforced per-query via added_this_query >= m rather than globally via out.len() >= m. For multi-query search_many calls, each query can contribute up to max_places entries, so the total result set can exceed max_places. This isn't introduced by this PR, but it's worth a follow-up issue if you'd like to track it.

---

Overall: the refactor is safe (behaviour-unchanged), the docs are accurate, and the code is strictly simpler. Ready to approve.

Would you like me to open a follow-up issue for the per-query vs. global max_places cap in non-enrich mode?

[approve]