feat: migrate DNS-mode ad-block onto master (#130)

[flyhigher139]

Summary

Migrate the DNS-mode ad-block feature (issue #130 + follow-ups #131, #132, #133, #134, #135, #138) from the long-lived orphan-ad-block-dns-bugfix branch onto a fresh base of master. The source branch also bundled extensive DNS hardening fixes (#140–#152, TCC nonce, Quick Apply, Terminal Native UI redesign, profile reapply on update) that this PR deliberately excludes — those remain unfixed on the source branch and are out of scope here.

The migration is split into 9 commits so each compiles independently and git blame traces each layer to its origin commit / PR.

What's new

• mhost-core: AdBlockResponse / AdBlockSource / AdBlockState types + 6 unit tests. SourceId / RuleSource::AdBlock were already dormant on master.
• mhost-storage: adblock.json + adblock-cache/{id}.txt persistence with corrupt-file backup-on-load (PR feat(adblock): add DNS-mode ad blocking #131 review finding 0.2).
• mhost-dns: AdBlockEngine with Arc::swap atomic rule publication (issue AdBlockEngine reload race: replace AtomicUsize short-circuit with Arc::swap publication #132). matcher::walk_parents shared with RuleEngine. QueryResult::NxDomain variant + build_nxdomain_response. reload_ad_block_rules clears the LRU cache so stale upstream IPs don't override new ad-block hits.
• AppState: 3 new fields (ad_block_state / ad_block_refresh_task / ad_block_refresh_cancel) + lock_or_recover poison helper + cold-start auto-recovery hook in AppState::new that re-publishes cached rules when dns_enabled=true was recovered from manifest (PR feat(adblock): add DNS-mode ad blocking #131 review P1-1).
• commands/dns.rs: 3 lifecycle helpers (spawn_ad_block_refresh_task / cancel_ad_block_refresh_task / abort_ad_block_refresh_task). Enable-time and disable-time hooks for the refresh task. The enable path does immediate classify_rules + reload_ad_block_rules before spawning the periodic refresh (fixes the orphan branch's gap where users with auto_refresh_enabled=false saw no rules until the first tick).
• commands/adblock.rs: 13 IPC commands + classify_rules / parse_blocklist_domains / persist_and_reload / fetch_sources_concurrent. Shared reqwest::Client via OnceLock. Bounded concurrent refresh (4 sources).
• Frontend: dedicated /ad-block route with master switch, sources CRUD, whitelist editor, auto-refresh controls, DNS-off banner. 14 Vitest cases. 13 typed IPC wrappers + 11 Jotai action atoms + 5 new base/derived state atoms.
• Tray: the previously-disabled "广告屏蔽（即将推出）" item is now enabled, emits a navigate event with "/ad-block", and the frontend route handler deep-links into the page.

What's NOT in this PR

Excluded by design — these are the DNS hardening bugs the user could not fix on the source branch. They are NOT introduced here:

• DNS hardening series: issues [Bug] 启用 DNS Mode 后，任何域名都 ping 不通了 #140, regression(#141): enable DNS mode hangs, no authorization dialog appears #142, fix: ship mhost-dns-proxy sidecar in .app + kill orphan proxy on enable + robust Ctrl+C exit #146, fix(dns): enable_dns_mode must kill orphan proxy via sudo + add AppleScript trap #148, feat(dns): Settings cancel button + IPC-level abort signal for DNS enable/disable #149, [Bug] DNS disable 不还原系统 DNS（DhcpEmpty 路径下网络被卡在 127.0.0.1） #152
• TCC nonce fix (commit 2df43ab on source branch)
• OriginalDns semantic refactor (issue [Bug] 切换 WiFi 后，DNS 模式下的 upstream 没有调整 #103 — already on master)
• Quick Apply (Follow-up: add safe quick-apply policy and structured apply feedback #127) + Terminal Native UI redesign (style(ui): apply Terminal Native visual refresh (Direction A) #125) + profile reapply ([Bug] 编辑当前激活的 hosts profile 时，只是保存了，但没写入 hosts 文件 #121)
• tokio::select! cancellation machinery in commands/dns.rs (issue feat(dns): Settings cancel button + IPC-level abort signal for DNS enable/disable #149)
• RUNTIME_DIR_TEST_LOCK in mhost-dns/lib.rs (issue fix(dns): enable_dns_mode must kill orphan proxy via sudo + add AppleScript trap #148 testing)
• tauri.conf.json externalBin: bin/mhost-dns-proxy sidecar wiring (issue regression(#141): enable DNS mode hangs, no authorization dialog appears #142)
• AppState::dns_cancel field (issue feat(dns): Settings cancel button + IPC-level abort signal for DNS enable/disable #149) — verified absent

CI clippy fix included

Last commit e7f862b fixes 7 pre-existing clippy errors on master that block CI:

• await_holding_lock (3 occurrences in proxy.rs tests) — intentional test_lock() serialization across .await for tests sharing filesystem state; added #[allow] with explanatory comment. Dropping the lock caused test_check_shutdown_signal + test_read_original_dns_from_file to fail intermittently under parallel execution.
• redundant_pattern_matching (2 occurrences in proxy.rs) — while let Ok(_) = x.await {} → while x.await.is_ok() {}.
• unused_variables (2 occurrences) — _start / _server prefix.

Verified by git checkout master -- src-tauri/ && cargo clippy ... reproducing the exact same 7 errors. The fix is required for the PR to pass CI's cargo clippy -D warnings gate.

Test plan

Already automated (each runs green locally):

• cargo test --workspace --lib --all-features: 408 passed
• cargo fmt --all -- --check: clean
• cargo clippy --workspace --all-targets --all-features -- -D warnings: clean
• pnpm test: 255 passed across 22 test files
• pnpm build: clean (tsc + vite)

Manual smoke (to be exercised by reviewers — macOS-only):

1. pnpm tauri dev → Settings → toggle DNS mode ON
2. Tray → "广告屏蔽" menu item navigates to /ad-block
3. Add source: name Someone Who Cares, url https://someonewhocares.org/hosts/hosts, response 0.0.0.0
4. dig @127.0.0.1 -p 1053 doubleclick.net A +short → expect 0.0.0.0
5. Add whitelist example.com; dig ... ads.example.com → expect real IP (not blocked)
6. Toggle DNS mode OFF → no stale processes; restart with dns_enabled=true from manifest → engine pre-populated from cache, first query already blocked (validates cold-start recovery)

Migration notes

• tokio-util = "0.7" added to [workspace.dependencies] and the mhost crate's [dependencies] (not mhost-dns — only the top-level package uses CancellationToken).
• mhost_storage::atomic_write made pub(crate) so the new adblock module can reuse it.
• RuleSource::AdBlock variant was already on master (dormant in models.rs:190); this PR only adds the AdBlock* types that consumers need.
• The dns_cancel field on AppState (issue feat(dns): Settings cancel button + IPC-level abort signal for DNS enable/disable #149) is intentionally NOT introduced — the orphan branch added it but we explicitly excluded feat(dns): Settings cancel button + IPC-level abort signal for DNS enable/disable #149 hardening work.

🤖 Generated with Claude Code

[flyhigher139]

PR #154 Code Review

Branch: feat/ad-block-dns → master
Size: +4026 / -32 across 31 files (9 commits)
Scope: migrate DNS-mode ad-block (issue #130 + follow-ups #131–#138) from orphan-ad-block-dns-bugfix onto a fresh master base, deliberately excluding the DNS-hardening series (#140, #142, #146, #148, #149, #152).
CI: Rust Checks ✓, Frontend Checks ✓, Detect changes ✓, Tauri Build Smoke (skipped — no externalBin change).

Overall: solid, well-tested migration. The persistence + engine + IPC layering is clean, the Arc::swap publication (issue #132), cold-start recovery (PR #131 review P1-1), persist-on-fetch-failure (P1-2), corruption-backup-with-collision-counter (finding 0.2), shared reqwest client (1.8 + 1.9), bounded concurrent refresh (1.4), and cancel-before-abort strategy (issue #138) are all implemented correctly and verified by the tests I inspected. No blockers, but several P1 issues below should land before merge — most stem from the new AdBlock.tsx page, not the Rust core.

---

1. Correctness

[P1] AAAA/A family mismatch returns NoError, defeating block intent

File: src-tauri/crates/mhost-dns/src/server.rs:683-702 (the handle_address_query ad-block ZeroAddress branch)

When the user has only IPv4 rules but queries AAAA, the engine hits and the (qtype=AAAA, ip=V4) arm falls through to QueryResult::NoError. Per RFC 1035 §4.1.1 / RFC 2308, NoError + empty answer means "name exists, no records of this type". The block intent is silently lost — AAAA queries to blocked domains will leak to upstream.

Fix: when ad-block matches but qtype/IP family don't match, return QueryResult::NxDomain (or empty NoError AA=1, but NxDomain is more defensible since the block source is itself a guarantee that this name should not resolve). Same applies symmetrically to (qtype=A, ip=V6).

[P1] AdBlock.tsx uses hash navigation while the app uses path routes

File: src/pages/AdBlock.tsx:3697 (the "Open Settings" CTA inside the DNS-off banner)

onClick={() => (window.location.hash = "#/settings")}

App.tsx registers <Route path="/settings" element={<Settings />}/> — react-router-dom v6 does not match hash paths. Setting window.location.hash puts #/settings in the URL but the router doesn't pick it up; the user sees no navigation.

Fix: import useNavigate and call navigate("/settings").

[P1] window.confirm() for source removal

File: src/pages/AdBlock.tsx:3887

if (confirm(`Remove source "${src.name}"?`)) { ... }

window.confirm() works in Tauri webview but is synchronous, blocking, and styling can't be themed. The rest of the app uses ManagementDrawer patterns — should follow the existing dialog convention (ApplyConfirmDialog etc.) for consistency, dark-mode, and a11y.

[P1] App.tsx and AdBlock.tsx missing trailing newline

File: src/App.tsx:3234, src/pages/AdBlock.tsx:4017 — diff shows \ No newline at end of file for both. pnpm build (tsc + vite) is clean per CI, but prettier --write would fix the trailing newline. Cosmetic but inconsistent with the rest of the codebase.

[P2] Over-broad #[allow(clippy::await_holding_lock)] in proxy.rs test

File: src-tauri/crates/mhost-dns/src/proxy.rs:644-660 (test_proxy_shutdown_signal_during_init)

The PR removed the original drop(_lock) line and added #[allow(clippy::await_holding_lock)]. The lock is only needed to serialize setup (writing shared filesystem state). The subsequent tokio::time::sleep(1500ms) doesn't touch shared state. Restore the original pattern:

let _lock = test_lock();   // serialize setup
// ... setup ...
let proxy_handle = tokio::spawn(...);
drop(_lock);               // release before the await-only phase

tokio::time::sleep(...).await;

The justification comment is right that the lock is intentional for setup — but the lint suppression should match that intent (one per affected test is fine if the lock genuinely spans .await; for this one it doesn't need to).

[P2] Tray navigate listener accepts arbitrary routes

File: src/App.tsx:43-48

const unlistenNavigate = listen<string>("navigate", (event) => {
  const target = event.payload;
  if (typeof target === "string" && target.startsWith("/")) {
    navigate(target);
  }
});

startsWith("/") is too permissive. The only known emitter today is the tray's "广告屏蔽" item emitting /ad-block. If a future tray entry or test pushes /__debug__ or a path the router doesn't know, the navigate happens silently (no-op render). Whitelist against ["/ad-block"] (and any future route explicitly registered) — refuse unknown payloads with a logged warning. Future-proofs against accidental internal-route exposure.

[P2] Inline styles inside AdBlock.tsx despite the dedicated module CSS

File: src/pages/AdBlock.tsx — multiple places: lines 3711, 3725, 3848, 3962, 3967, 3991, 4002

style={{ display: "flex", justifyContent: "space-between", ... }} appears at least 6 times, despite the PR adding 190 LOC of AdBlock.module.css. Move these to CSS classes (e.g. .sourceHeaderRow, .addSourceForm already exists, .inlineForm already exists). The CSS file is otherwise well-organized — don't leak inline styles back in.

[P2] Whitelist input lacks domain-syntax validation

File: src-tauri/src/commands/adblock.rs:2278 (add_ad_block_whitelist)

let normalized = domain.trim().to_lowercase();
if normalized.is_empty() { ... }

That's the only check. walk_parents does literal HashSet::contains — entries like *.example.com, example.com/path, not a domain at all, or whitespace are persisted silently and never match. They also don't surface in last_error so the user has no signal that the entry is broken.

Fix: validate against a simple domain regex (e.g. ^[a-z0-9._-]+$, no leading/trailing dot, no consecutive dots) — return MhostError::InvalidInput on bad input. Reject entries longer than 253 chars (RFC 1035 max).

[P3] onPointerDown={onPointerDown(() => {})} repeated 10+ times

File: src/pages/AdBlock.tsx (Refresh all, Open Settings, Add, Refresh, Delete, Add whitelist)

The empty () => {} handler looks like it's only here to call the hook. If useWebKitPointerDown is meant to suppress WebKit quirks, consider a <PointerDownSuppressedButton> wrapper or onPointerDown={onPointerDown()} to avoid the per-button closure.

---

2. Error handling & robustness

[P2] spawn_ad_block_refresh_task is called when dns_enabled=true is loaded but ad-block state still being parsed

File: src-tauri/src/state/mod.rs:3027-3047 (the cold-start block in AppState::new)

The cold-start path calls classify_rules synchronously inside AppState::new while holding nothing — but classify_rules is sync and reads cache files. If storage.root() is on a slow filesystem (network mount, encrypted APFS), this blocks AppState::new's completion, which is called from setup on the main thread. The comment doesn't address this.

Fix: wrap the cold-start reload in spawn_blocking and pass a oneshot to the task. The current code is fine on local SSD; this is defensive.

[P3] set_ad_block_refresh_interval clamps to 0..168 but 0 triggers no task spawn

File: src-tauri/src/commands/adblock.rs:2062

hours.clamp(0, 24 * 7) — 0 is valid (disables auto-refresh). Fine, but the frontend select sends 0 via "Manual only", and spawn_ad_block_refresh_task short-circuits when cfg.0 || cfg.1 == 0. Documented in the helper, but worth a one-line test that 0 doesn't spawn.

[P3] fetch_sources_concurrent errors are eprintln!'d, not returned

File: src-tauri/src/commands/adblock.rs:2241

if let Err(e) = fetch_and_cache_source_internal(...).await {
    let _ = record_fetch_error_internal(...).await;
    eprintln!("[adblock] concurrent refresh source {} failed: {}", id, e);
}

The error is also recorded on the source's last_error (correct), so the UI badge catches it. But the eprintln! is the only place these errors go. Consider tracing::warn! to be consistent with build_nxdomain_response's tracing::warn!. The eprintln! on domains_for_source (line ~1824) has the same pattern.

[P3] tray.rs emit("navigate", ...) not awaited

File: src-tauri/src/tray.rs:228

let _ = window.emit("navigate", "/ad-block"); — Tauri 2's emit returns Result<()>. let _ is fine for fire-and-forget but should be a if let Err(e) = ... { tracing::warn! } like the surrounding lines, for parity with window.show() / set_focus().

---

3. Concurrency & cancellation

What's good

• Arc::swap rule publication (adblock.rs:339-358) — old Arc dropped outside the write lock, the canonical fix for the issue #132 0→N leak and the reviewer-flagged "Medium #1".
• AdBlockEngine::check empty fast-path (adblock.rs:374-378) — reads the published snapshot, not a stale AtomicUsize short-circuit. Fixes the issue #132 inconsistency window.
• walk_parents shared with RuleEngine (matcher.rs) — single source of truth for suffix-walk semantics across both engines; well tested.
• Cancel-before-abort (dns.rs disable path) — the cancel token lets select! wake immediately; spawn_blocking closures self-check is_cancelled() before mutating the server. Plus the token-swap-on-spawn fix for the re-enable-after-disable cycle. Issue #138 is genuinely closed.
• Cold-start recovery (state/mod.rs:3027-3047) — verified wired correctly; re-publishes cached rules + spawns refresh task when dns_enabled=true is recovered from manifest.
• 3 test AppState literals in commands/dns.rs — verified they have the 3 new ad-block fields added (PR commit message said "added in commit message only … commit 5 dropped it on the way through" — diff confirms the fields are present at lines 2661-2663, 2673-2675, 2683-2685).
• fetch_sources_concurrent bounded at REFRESH_CONCURRENCY=4 — semaphore pattern, permits released on task drop. Serial loop → 4× faster for typical lists, per PR #131 review finding 1.4.

[P2] cancel_ad_block_refresh_task doesn't wait for the task

File: src-tauri/src/commands/dns.rs:2714 (abort_ad_block_refresh_task)

fn abort_ad_block_refresh_task(slot: &Mutex<Option<JoinHandle<()>>>) -> bool {
    if let Some(handle) = lock_or_recover(slot).take() { handle.abort(); true }
    else { false }
}

This is acknowledged in the helper's docstring as "a separate concern tracked as a follow-up issue" — but the implication is that set_dns_mode_disable can return Ok(()) while the refresh task is still mid-spawn_blocking. The disable path then proceeds to mhost_dns::platform::disable_dns_mode which kills the server. If the spawn_blocking closure checks is_cancelled() before calling reload_ad_block_rules (which the code does, at the persist_and_reload boundary), this is safe. But if there's a window between the cancel check and the actual reload_ad_block_rules call where the server has been .take()n out and dropped, the closure will read a dangling Arc<None>.

Actually: looking more carefully — reload_ad_block_rules is called via server: &Arc<...> (state.dns_server), and the disable path only drops dns_server: Arc<Mutex<Option<DnsServer>>> after the network teardown. So the Arc itself stays valid (the inner Option becomes None). The race window is bounded. Not a blocker, but the docstring understates the risk — the cancel + abort is correct only because Arc<Mutex<Option<DnsServer>>> keeps the outer Arc alive even after take().

---

4. Security & boundaries

What's good

• URL scheme check (adblock.rs:2107) — only http:// / https://. No file:// / ftp:// SSRF vector.
• Size enforcement (adblock.rs:1641-1666) — two-stage guard (Content-Length pre-check + post-read size). PR #131 finding 1.8 closed.
• SourceId path-traversal debug_assert (storage/adblock.rs:1240) — defensive !contains('/') && !contains('\\').
• No raw fs::write + rename — every write goes through atomic_write (CLAUDE.md invariant).
• Capability manifest — no change. CSP / permissions intact.
• DNS server still bound to 127.0.0.1 — no networking surface added.

[P3] Whitelist / source URL — no max-length check

add_ad_block_source validates non-empty + http(s). A user could submit a 10MB URL string. Realistically bounded by the IPC layer, but worth a MAX_URL_LEN = 2048 check. Same for whitelist entries (MAX_DOMAIN_LEN = 253 per RFC 1035).

[P3] MAX_RULES_PER_SOURCE = 100_000 enforced at parse, but cache files are unbounded on disk

If a user adds a source with 80k rules, removes it (cache purged via purge_source), and adds it again with 90k rules — the new write is atomic. Fine. But if MAX_RULES_PER_SOURCE is bumped in a future release, an old cache file from a more permissive version would be loaded as-is. Add a versioned cache header (e.g. adblock-cache/v1/{id}.txt) so future format changes can reject stale caches cleanly.

---

5. Performance

What's good

• spawn_blocking for sync IO + parsing (persist_and_reload, fetch_and_cache_source) — issue #133 closed: 100k+ domain parsing no longer starves concurrent DNS queries.
• Arc::swap snapshot for hot reload — readers take the lock only for the Arc refcount bump, then walk the snapshot lock-free. No more 0→N leak / blocking reload.
• LRU cache cleared on reload_ad_block_rules (server.rs:355) — issue #132 follow-up closed: stale upstream IPs don't shadow new ad-block hits.
• adBlockRuleCountAtom filters enabled — the atom recomputes on state change; correct scope (only enabled sources contribute).

[P2] 100k × N String allocations on every reload

File: src-tauri/src/commands/adblock.rs:1826 (parse_blocklist_domains)

for d in rule.domains {
    domains.push(d.to_lowercase());
}

Each iteration allocates a new String. For 100k domains × 4 sources that's 400k String allocations per refresh tick (every 1-24h). The domains Vec is then iterated again in classify_rules to populate HashMap<String, IpAddr>. Consider Arc<str> (cheap to clone) or Box<str>. Minor — the work is in spawn_blocking so it doesn't block DNS queries, but for a 5-minute interval with a 200k-rule list, GC pressure adds up.

[P3] adBlockRuleCountAtom doesn't update atomically on state mutation

The atom is a derived getter over adBlockStateAtom. When the user toggles a source, the source list mutates → adBlockStateAtom changes → atom recomputes. Good. But the displayed ruleCount.toLocaleString() drops when a source is disabled — users may think rules were deleted. Suggestion: show "Active: 95,000 / Total: 195,000" in the UI.

---

6. Tests & CI

What's good

• 408 Rust tests + 255 Vitest tests pass per PR description.
• 6 storage unit tests + read_state_or_default_* backup tests (storage/adblock.rs:1350-1542) — covers missing file, round-trip, atomicity (no stray tmp), cache CRUD, idempotent delete, corruption backup, collision counter.
• PR #131 regression tests (commands/adblock.rs:2488, 2529) — classify_rules_populates_from_cached_source (P1-1) and add_ad_block_source_persists_on_fetch_failure (P1-2) explicitly cover the two re-review findings.
• 14 Vitest cases on AdBlock.test.tsx — covers loading state, master switch, sources CRUD, whitelist, refresh interval, error display.
• AdBlockEngine unit tests (adblock.rs:431-522) — empty engine, fast-path, ZeroAddress hit, NXDOMAIN hit, whitelist precedence over both.

[P2] No test exercises the "DNS-off then add source" path

The set_dns_mode_enable enable-time hook + AppState::new cold-start hook both call classify_rules then reload_ad_block_rules. There's a test for cold-start (classify_rules_populates_from_cached_source), but no test for "DNS mode is OFF, user adds a source → reload doesn't fire" or "DNS mode goes OFF → ON → first query sees cached rules immediately". The first scenario is covered implicitly by add_ad_block_source_persists_on_fetch_failure (since dns_enabled=false in tests), but the latter (cold-start hot-reload to a fresh server) is the headline PR #131 P1-1 fix and deserves an integration test.

[P3] No test exercises walk_parents from a registered TLD

The matcher.rs test walk_parents_visits_single_label_parent_once verifies "com" is hit. Pi-hole-style TLD blocking is documented as supported but not unit-tested. Add a test that walk_parents("foo.bar.com", |d| ... "com" matches ...).

---

7. Migration correctness

What's good — exclusions verified

The PR description lists exclusions for the DNS hardening series. I verified each by grepping the diff:

• ✅ No dns_cancel field on AppState (issue #149 excluded)
• ✅ No externalBin / mhost-dns-proxy sidecar wiring (issue #142 excluded)
• ✅ No OriginalDns refactor (issue #103 follow-up — already on master)
• ✅ No tokio::select! machinery in commands/dns.rs (issue #149)
• ✅ No RUNTIME_DIR_TEST_LOCK (issue #148 testing)

[P2] Clippy fix patch in commit e7f862b is required for CI but pre-existing

The commit fixes 7 clippy 1.96 lints on master baseline — await_holding_lock (3 tests), redundant_pattern_matching (2), unused_variables (2). These should ideally land as a separate, smaller PR (fix(clippy): …) before this large migration to keep git history clean. Bundling makes the migration hard to revert if the clippy fixes have regressions.

Not a blocker, but a process improvement: split the clippy commit into its own PR, rebase this one on top.

[P3] mhost_storage::atomic_write made pub(crate)

File: src-tauri/crates/mhost-storage/src/storage.rs

The visibility bump from private to pub(crate) is fine (the new adblock module needs it), but pub(crate) exposes it to all crates in the workspace. If you'd rather keep the surface tight, consider a thin pub(crate) fn write_file(path: &Path, bytes: &[u8]) wrapper that delegates to atomic_write, exposing only the cross-module use case. Cosmetic.

---

8. Frontend

What's good

• 13 typed IPC wrappers (src/lib/tauri.ts) — return types match the Rust command signatures; snake_case ↔ camelCase mapping is consistent (source_id, refresh_interval_hours, auto_refresh_enabled).
• Type ↔ Rust model mirrored (src/types/index.ts:48-78) — comment explicitly notes the wire format is nx_domain per serde(rename_all = "snake_case"); verified against models.rs:146-153 test that asserts "\"nx_domain\"".
• DNS-off banner with "Open Settings" CTA — good UX for the common first-time flow.
• Atoms well-scoped — adBlockStateAtom / isAdBlockLoadingAtom / adBlockErrorAtom / adBlockRuleCountAtom / adBlockHasErrorsAtom. Mutating actions go through persist_and_reload (single source of truth).
• App.tsx tray listener registration — correct, both events (tray:profiles-updated, navigate) registered with proper cleanup in the effect's return.
• /ad-block route registered in App.tsx.

[P2] App.test.tsx lost its toHaveBeenCalledTimes(1) assertion

File: src/App.test.tsx:60 and :107

- expect(listenMock).toHaveBeenCalledTimes(1);
  expect(listenMock).toHaveBeenCalledWith("tray:profiles-updated", expect.any(Function));
+ expect(listenMock).toHaveBeenCalledWith("navigate", expect.any(Function));

The PR correctly adds the second toHaveBeenCalledWith but drops the total count assertion. The test no longer catches the case where App registers the profiles-updated listener twice. Either keep toHaveBeenCalledTimes(2) or test each listener registration with toHaveBeenCalledWith.

[P3] AdBlock.tsx lacks i18n

"Ad Block", "Loading…", "Open Settings", "Refresh all", "Sources", "Add", "Whitelist", "Auto-refresh" — all hardcoded English. The PR description notes Terminal Native UI redesign (#125) is excluded, but does the rest of the app have i18n at all? If yes, this page should match. If no, this is consistent with current state.

[P3] confirm() blocks webview thread

Already mentioned in §1. Tauri 2 webview typically supports window.confirm() but it's synchronous — the entire Tauri IPC bridge stalls while the dialog is up. For a rarely-used action like source deletion this is fine; for a 4-source list where the user might click "Delete" four times in a row it's annoying. Use a custom dialog.

---

Summary

Severity	Count	Headline
P0 blocker	0	—
P1 important	4	AAAA/AAAA family mismatch; window.location.hash; confirm(); trailing newlines
P2 worth fixing	7	Over-broad await_holding_lock allow; whitelist syntax check; tray route whitelist; inline styles; clippy commit bundling; cold-start integration test; App.test count assertion
P3 nit	6+	Domain validation depth; max-lengths; tracing::warn! consistency; pub(crate) scope; ruleCount UX; i18n; matcher TLD test

Recommendation: ✅ Approve with P1 fixes before merge. The Rust core is solid and well-tested — the issues above are concentrated in the new frontend page and the over-broad lint suppressions. The P1 AAAA/AAAA family mismatch is the only one that affects user-visible behavior (ad-block not blocking AAAA queries on IPv4-only lists); the rest are UX/polish.

---

Verified during review

• AppState::new cold-start recovery (state/mod.rs:3027-3047) — wired correctly ✓
• 3 test AppState literals in commands/dns.rs — fields present at lines 2661-2685 ✓
• dns_cancel field NOT introduced (issue #149 excluded) ✓
• externalBin sidecar wiring NOT introduced (issue #142 excluded) ✓
• lock_or_recover uses into_inner() — standard poison recovery ✓
• Arc::swap publication drops old snapshot outside write lock ✓
• walk_parents visit order: a.b.example.com → b.example.com → example.com → com ✓
• fetch_source two-stage size enforcement (Content-Length + post-read) ✓
• cancel-before-abort ordering on set_dns_mode_disable ✓
• tokio_util::sync::CancellationToken::new() swap on every spawn (re-enable fix) ✓

[flyhigher139]

PR #154 Re-review (after commit 6daffa5a)

Diff after fix: +4156/-31 across 31 files (10 commits, +1 since v1)
Scope of fix commit: 4 P1 + 4 P2 items addressed in 6daffa5a fix(ad-block): address PR #154 review feedback
Verified locally (via cargo clippy ... -D warnings, cargo fmt --check, cargo test --workspace --lib, pnpm test, pnpm build — all per commit message).

Headline: 7 of 8 items fixed correctly. One P1 regression introduced (visual layout in bannerText CSS class — master switch toggle now stacks under description instead of sitting beside it). One new P2 gap (validate_whitelist_domain has no tests). Recommendation: ship the P1 CSS fix and add at least one validation test, then merge.

---

What was fixed correctly ✓

P1 #1 — AAAA/A family mismatch (server.rs:693-704)

// qtype 与规则 IP family 不匹配（如 AAAA 命中 IPv4 规则）：
// 返回 NxDomain 而非 NoError。NoError + 空答案在 RFC 2308
// 语义里是「name 存在但无此 type 记录」，等于放行让
// 上游继续解析 —— 把广告屏蔽的意图完全绕开了。
_ => return QueryResult::NxDomain,

Correct: both the family-mismatch arm and the defensive None => QueryResult::NxDomain fallback preserve block intent. Comment cites RFC 2308 — block semantics now match Pi-hole behavior.

P1 #2 — Hash → useNavigate (AdBlock.tsx)

const navigate = useNavigate();
...
onClick={() => navigate("/settings")}

Correct. Matches the app's path-based router.

P1 #3 — window.confirm() → @tauri-apps/plugin-dialog.confirm (AdBlock.tsx)

confirmDialog(
  `Remove source "${src.name}"?`,
  { title: "Remove Source", kind: "warning" },
).then((ok) => { if (ok) removeSource(...).catch(() => {}); }).catch(() => {});

Async, native, dark-mode + a11y consistent with the rest of the app.

P1 #4 — Trailing newlines

App.tsx ends with \n, AdBlock.tsx ends with \n. ✓

P2 #5 — Narrowed await_holding_lock scope (proxy.rs)

Verified final state (lines 514/554/732 keep #[allow]; line 632 test_proxy_shutdown_signal_during_init no longer has it). The 3 remaining allows all genuinely need the lock across .await. The drop(_lock) between setup and the await-only phase is correct — the verification sleep(1500ms) no longer holds the lock.

P2 #6 — ALLOWED_TRAY_ROUTES whitelist (App.tsx:53-61)

const ALLOWED_TRAY_ROUTES: ReadonlySet<string> = new Set(["/ad-block"]);
const unlistenNavigate = listen<string>("navigate", (event) => {
  const target = event.payload;
  if (typeof target === "string" && ALLOWED_TRAY_ROUTES.has(target)) {
    navigate(target);
  } else if (typeof target === "string" && target.startsWith("/")) {
    console.warn(`[mHost] tray navigate: refused unknown route "${target}"`);
  }
});

Clean — explicit allow-list with a console warning for rejected paths. Future-proofs against stray internal paths being pushed by a debug/test hook.

P2 #7 — validate_whitelist_domain (commands/adblock.rs:272-305)

The validation logic is sound:

• whitespace → reject
• / or \ → reject
• leading . → reject
• * → reject (suffix-walk already covers subdomain match)
• non-[a-z0-9.-] → reject

remove_ad_block_whitelist correctly skips validation with an explanatory comment ("matches the contract of 'remove what matches; ignore the rest'").

---

⚠️ Regression introduced — must fix before merge

[P1] bannerText CSS class is wrong — master switch toggle now stacks vertically instead of sitting beside the label

File: src/pages/AdBlock.module.css:191-197 (the new .bannerText class)

The original inline style on the master switch row was:

style={{
  display: "flex",
  justifyContent: "space-between",   // row, label left + toggle right
  alignItems: "center",
  gap: 12,
}}

The new CSS class is:

.bannerText {
  flex: 1;
  display: flex;
  flex-direction: column;            // ← wrong direction
  gap: 2px;                          // ← wrong gap
}

The class is applied to the master switch card container at AdBlock.tsx:152 (where the label "Enable Ad Block" / description and the <label className="toggle"> are siblings). With the new CSS the toggle will stack below the description text instead of sitting to the right of it.

Suggested fix — either rename to match the actual semantic (.rowSpaceBetween) or correct the values:

.bannerText {
  display: flex;
  justify-content: space-between;
  align-items: center;
  gap: 12px;
}

Also worth noting: the class name bannerText is misleading because there's already a .banner class used for the orange "DNS mode is off" alert (line 138 of the TSX). Consider renaming to avoid future confusion — .masterSwitchRow or .rowSpaceBetween would be clearer.

---

Incomplete — should also fix before merge

[P2] validate_whitelist_domain has zero test coverage

File: src-tauri/src/commands/adblock.rs:272-305 (the new function)

grep validate_whitelist on the file shows only the function definition and the call site — no #[test] blocks. The function has 6 rejection branches (empty / whitespace / / / \ / leading . / * / non-ASCII) and one happy path. Worth at least 4 tests:

• happy path: " Example.COM " → "example.com"
• rejection: *.example.com, example.com/path, not a domain, empty string
• reject-then-save: confirm add_ad_block_whitelist returns Err(InvalidInput) and does NOT mutate state

This is the kind of helper that gets refactored and breaks silently — a 30-line test block prevents regressions and is required for PR-review hygiene.

[P3] AdBlock.module.css still missing trailing newline

File: src/pages/AdBlock.module.css — file ends with } and no \n (xxd confirms last bytes are ... 2px 6px;}, no terminating newline). Was already true in v1; fix commit touched this file but didn't add the newline. prettier --write would fix.

---

What's still left from v1 (lower priority, optional for merge)

[P3] remove_ad_block_whitelist ignores * entries silently

Intentional per the inline comment ("matches the contract of 'remove what matches; ignore the rest'"). If a user pastes *.example.com to remove the entry they added earlier, the request no-ops with no signal. Acceptable for the merge but worth a UX hint in the toast ("Removed: api.example.com" with a count, or a "nothing matched" warning).

[P3] confirmDialog not disabled while pending

The new async confirmDialog().then(...) doesn't disable the Delete button while the dialog is up. A user clicking Delete repeatedly on a 4-source list will queue 4 dialogs. Add a local useState<boolean> pendingDeleteId and disable the per-row button while one is open.

[P3] onPointerDown={onPointerDown(() => {})} still repeated 10+ times

My v1 nit, not addressed. If useWebKitPointerDown exists to suppress WebKit pointer quirks, the per-button closure is a code smell. Consider a wrapper component or onPointerDown={onPointerDown()}.

[P3] Frontend i18n not addressed

All strings in AdBlock.tsx are still hardcoded English. Wasn't blocking in v1; still not blocking.

---

Summary

Item	v1	v2 (this commit)
AAAA family mismatch	P1	✅ Fixed (NxDomain)
Hash navigation	P1	✅ Fixed (useNavigate)
window.confirm	P1	✅ Fixed (Tauri dialog)
Trailing newlines	P1	✅ Fixed for App.tsx + AdBlock.tsx
over-broad await_holding_lock	P2	✅ Fixed (lock dropped)
Tray route whitelist	P2	✅ Fixed (allow-list + warning)
Inline styles → CSS classes	P2	⚠️ 8 of 9 fixed correctly; bannerText class is the wrong layout
Whitelist domain validation	P2	✅ Fixed logic, no tests added
Clippy fix commit bundling	P2	(still bundled; not blocking)
Cold-start integration test	P2	(not added; recommend follow-up issue)
App.test.tsx count assertion	P2	(still missing)
confirm() UX polish	P3	(improved but not perfect)

Recommendation: ✅ Approve with two blockers addressed:

1. Fix bannerText CSS class to match original display: flex; justify-content: space-between; align-items: center; gap: 12px (rename to avoid confusion with .banner)
2. Add at least 3 #[test] cases for validate_whitelist_domain (happy path + reject-* + reject-/path)

Optionally also fix the CSS trailing newline (one-line .git add -p).

Everything else from v1 is either addressed or acceptable-as-nits for a follow-up issue.

---

Re-verified during this re-review (line numbers from current feat/ad-block-dns)

• server.rs:693-704 — AAAA/AAAA family mismatch → NxDomain ✓
• App.tsx:53-62 — ALLOWED_TRAY_ROUTES whitelist + warning ✓
• App.tsx:87 — file ends with \n ✓
• AdBlock.tsx:439 — file ends with \n ✓
• proxy.rs:514/554/732 keep #[allow], :632 does not ✓
• commands/adblock.rs:272-305 validate_whitelist_domain exists, called at :797 ✓
• commands/adblock.rs:803-805 remove_ad_block_whitelist skips validation with comment ✓
• AdBlock.module.css ends with } no \n ❌ (still missing)
• bannerText CSS at AdBlock.module.css:191-197 is column/gap-2, applied to master switch row (TSX:152) ❌ (wrong layout)

[flyhigher139]

PR #154 Re-review (after commit 93cf95cc)

Diff: +4378/-32 across 31 files (11 commits, +1 since v2)
Scope of fix commit: addressed 2 P2 + 3 P3 + 1 no-op from the round-1 review's lower-priority items (cold-start spawn_blocking, App.test count assertion, max-length guards, TLD test, tray.rs emit logging, parse_blocklist no-op).

Headline: v1 leftovers all addressed correctly — including an excellent cold_start_hot_reload_blocks_first_query integration test that fires a real UDP query through a real DnsServer. However, the two blockers from my round-2 review (bannerText CSS regression + validate_whitelist_domain lacking tests) are still present. They were not picked up in this commit. Below I re-confirm both issues with current-file evidence and outline what would close them.

Recommendation: Land the two small blockers (5-line CSS fix + ~40 lines of validation tests), then merge. Everything else is good.

---

What the new commit fixed correctly ✓

P2 #1 — App.test.tsx re-adds count assertion (with StrictMode awareness)

File: src/App.test.tsx:60-67, 117-118

// PR #154 review (P2): keep total call count so a double-registration
// regression is caught. We register 2 listeners per mount: `tray:profiles-updated`
// (profile refresh) and `navigate` (issue #130 tray deep-link). React
// StrictMode in dev causes double mount → 2 × 2 = 4 calls. Use `>=` to
// tolerate the StrictMode double-mount while still catching obvious
// regression cases (e.g. a third registration).
expect(listenMock.mock.calls.length).toBeGreaterThanOrEqual(2);

The >= instead of === is the right call — StrictMode in dev genuinely double-mounts. The test still catches "someone added a 5th registration" or "one of the two was deleted" (in the latter case, count drops to 2 mounts × 1 listener = 2, which is the boundary, so technically it would still pass — a follow-up nit). Solid for now.

P2 #2 — Cold-start classify_rules moved into spawn_blocking

File: src-tauri/src/state/mod.rs:188-219

if state.dns_enabled.load(Ordering::Relaxed) {
    let snap = state.ad_block_state.read().await.clone();
    let storage_root = state.storage.root().to_path_buf();
    let dns_server = Arc::clone(&state.dns_server);
    let result = tokio::task::spawn_blocking(move || {
        let (za, nx, wl) = crate::commands::adblock::classify_rules(&snap, &storage_root);
        if let Some(server) = crate::state::lock_or_recover(&dns_server).as_ref() {
            server.reload_ad_block_rules(za, nx, wl);
        }
    })
    .await;
    if let Err(e) = result {
        eprintln!(
            "[mHost] cold-start ad-block reload join error: {} (continuing without hot-reload)",
            e
        );
    }
    crate::commands::dns::spawn_ad_block_refresh_task(...);
}

Correct — the sync IO + parse is the slow part and now runs on a blocking thread. The eprintln! on join-error is acceptable (we'd log the error and continue, leaving the server running with empty ad-block rather than failing startup). The spawn_ad_block_refresh_task call stays on the async runtime since it's purely async. The decision is well-commented.

Type-soundness verified: dns_server: Arc<Mutex<Option<mhost_dns::DnsServer>>> (confirmed at state/mod.rs:62); Mutex is std::sync::Mutex, and the spawn_blocking closure correctly uses crate::state::lock_or_recover.

P2 #3 — New integration test cold_start_hot_reload_blocks_first_query

File: src-tauri/src/commands/adblock.rs:1047-1174

#[tokio::test]
async fn cold_start_hot_reload_blocks_first_query() {
    // ... seed cache, build state, simulate cold-start classify_rules,
    //     wire into a real DnsServer, fire a real UDP query for the
    //     blocked domain, assert the response contains an A record
    //     with 0.0.0.0 — no upstream leak.
    ...
    assert_eq!(a.0, std::net::Ipv4Addr::new(0, 0, 0, 0),
        "cold-start ad-block should return 0.0.0.0");
}

This is the strongest test in the PR. It exercises the full cold-start path that v1 didn't have:

• Real DnsServer::new(config).unwrap()
• Real reload_ad_block_rules publication into the engine
• Real tokio::spawn of the listener
• Real UDP round-trip with tokio::net::UdpSocket
• Real hickory_proto DNS message encode/decode
• Assertion that the blocked domain returns 0.0.0.0 immediately

This is exactly the integration test I asked for in round 1 as a "follow-up issue" — it's now a real test. ★

Minor nit on pick_free_port (line 1169): the helper does bind("127.0.0.1:0") then immediately drop(listener) to "release" the port. On most OSes the port is released on drop, which means another process could grab it before the test binds. A safer pattern would be to keep the listener socket open (with SO_REUSEADDR) and pass it through. In practice, the polling loop with 10ms + 2s deadline would catch most races, but a cleaner approach is bind then pass the listener. P3.

P3 #4 — Max-length guards

File: src-tauri/src/commands/adblock.rs:48-53, 286-291, 612-617

const MAX_URL_LEN: usize = 2048;
const MAX_DOMAIN_LEN: usize = 253;
...
if trimmed.len() > MAX_DOMAIN_LEN {
    return Err(format!("whitelist entry length {} exceeds limit {}", ...));
}
...
if url.len() > MAX_URL_LEN {
    return Err(MhostError::InvalidInput(format!("source url length {} ...", ...)));
}

Both at the IPC boundary. The RFC 1035 limit (253 chars per domain, 63 per label) is correctly cited. Good.

P3 #5 — TLD walk test

File: src-tauri/crates/mhost-dns/src/matcher.rs:78-103

#[test]
fn walk_parents_registered_tld_matches_every_subdomain() {
    let r = walk_parents("deeply.nested.subdomain.example.com", |d| match d {
        "com" => Some("TLD-hit"), _ => None,
    });
    assert_eq!(r, Some("TLD-hit"));
    // negative case: .org does NOT match a com-only rule
    let r = walk_parents("example.org", |d| match d {
        "com" => Some("TLD-hit"), _ => None,
    });
    assert_eq!(r, None);
}

Covers the positive case (TLD registered → all subdomains match) AND the negative case (different TLD doesn't false-positive). Locks in Pi-hole semantics.

P3 #6 — tray.rs emit error logging

File: src-tauri/src/tray.rs:226-232

if let Err(e) = window.emit("navigate", "/ad-block") {
    tracing::warn!("[mHost] tray emit navigate failed: {}", e);
}

Parity with the surrounding unminimize/show/set_focus calls. Now consistent — every emit-on-tray path logs failures the same way.

No-op #7 — parse_blocklist_domains allocation analysis

File: src-tauri/src/commands/adblock.rs:323-336 (the new comment)

/// **PR #154 review (P2)**: no-op — after analysis, the original
/// `d.to_lowercase()` is correct and the only allocation we can avoid
/// here is for already-lowercase strings (the common case for
/// well-formed blocklists). The `eq_ignore_ascii_case` /
/// `to_ascii_uppercase` shortcut doesn't actually save allocations
/// (`to_ascii_uppercase` allocates a String) and breaks the
/// `MiXed.ExAmPlE.com → mixed.example.com` semantic that the
/// `parse_blocklist_lowercases` test relies on. Sticking with the
/// straightforward `to_lowercase()` — the work runs in
/// `spawn_blocking` (PR #131 P1-2 + issue #133), so DNS queries
/// aren't blocked during the parse.

Reasoned analysis, correct conclusion. The to_ascii_uppercase shortcut doesn't actually save allocations — it allocates a new String for the upper-case form, then eq_ignore_ascii_case does the comparison, then to_lowercase() allocates another String. So the "optimization" was a wash or worse. Documented well.

---

⚠️ Two blockers from round-2 — still NOT addressed

[P1, still] bannerText CSS class has the wrong layout

File: src/pages/AdBlock.module.css:191-197 — current state:

.bannerText {
  flex: 1;
  display: flex;
  flex-direction: column;
  gap: 2px;
}

Applied at: src/pages/AdBlock.tsx:152 — the master switch card row that contains the "Enable Ad Block" label, the description, and the <label className="toggle"> toggle.

What the original inline style was (from v1, before round-2):

style={{
  display: "flex",
  justifyContent: "space-between",   // row layout — label left, toggle right
  alignItems: "center",
  gap: 12,
}}

The new CSS breaks this: it stacks the toggle below the description text instead of placing it to the right.

Verified live: I fetched the file via GitHub API just now (AdBlock.module.css is 4025 bytes, last 50 chars 'badgeSm {\n font-size: 12px;\n padding: 2px 6px;\n}'); AdBlock.tsx is 16042 bytes, ends with newline; line 152 still has <div className={styles.bannerText}>. The bug is live in the current feat/ad-block-dns branch.

Suggested 5-line fix:

.bannerText {
  display: flex;
  justify-content: space-between;
  align-items: center;
  gap: 12px;
}

Or rename to rowSpaceBetween to avoid future confusion with the .banner (DNS-off orange alert) class.

[P2, still] validate_whitelist_domain has zero test coverage

File: src-tauri/src/commands/adblock.rs:280-321

I verified with grep against the current file: there are 5 #[test] blocks and 2 #[tokio::test] blocks in this file, but none reference validate_whitelist_domain. The function has 7 rejection branches + 1 happy path + the new MAX_DOMAIN_LEN check, all untested.

Suggested minimum coverage (~40 lines):

#[test]
fn validate_whitelist_domain_happy_path_lowercases_and_trims() {
    assert_eq!(validate_whitelist_domain("  Example.COM  ").unwrap(), "example.com");
}

#[test]
fn validate_whitelist_domain_rejects_empty() {
    assert!(validate_whitelist_domain("   ").is_err());
}

#[test]
fn validate_whitelist_domain_rejects_wildcard() {
    let err = validate_whitelist_domain("*.example.com").unwrap_err();
    assert!(err.contains("'*'"), "unexpected error: {}", err);
}

#[test]
fn validate_whitelist_domain_rejects_path() {
    assert!(validate_whitelist_domain("example.com/path").is_err());
    assert!(validate_whitelist_domain("example.com\\path").is_err());
}

#[test]
fn validate_whitelist_domain_rejects_whitespace_inside() {
    assert!(validate_whitelist_domain("foo bar.com").is_err());
}

#[test]
fn validate_whitelist_domain_rejects_leading_dot() {
    assert!(validate_whitelist_domain(".example.com").is_err());
}

#[test]
fn validate_whitelist_domain_rejects_oversize() {
    let huge = "a".repeat(254);
    assert!(validate_whitelist_domain(&huge).is_err());
}

#[test]
fn validate_whitelist_domain_rejects_unicode() {
    assert!(validate_whitelist_domain("例え.com").is_err());
}

---

What's still in flight (carried over from v1/v2, not blocking)

[P3] remove_ad_block_whitelist silently no-ops on invalid input

Intentional per the v2 comment ("matches the contract of 'remove what matches; ignore the rest'"). Acceptable.

[P3] confirmDialog doesn't disable the Delete button while pending

A user can queue multiple native dialogs by clicking Delete fast on a 4-source list. Add a local useState<Set<string>> pendingDeletes and disable per-row buttons while one is open.

[P3] onPointerDown={onPointerDown(() => {})} repeated 10+ times

Was a v1 nit. Not addressed. Wrap as <PointerDownSafeButton> or hoist the empty handler.

[P3] Frontend strings not i18n'd

AdBlock.tsx has hardcoded English throughout. Project-wide i18n is out of scope for this PR; the rest of the app is also English-only. Acceptable.

---

Summary

Item	Status
Cold-start spawn_blocking	✅ Fixed
App.test.tsx count assertion (with >= for StrictMode)	✅ Fixed
cold_start_hot_reload_blocks_first_query integration test	✅ Fixed (★ strongest test in the PR)
MAX_URL_LEN / MAX_DOMAIN_LEN guards	✅ Fixed
walk_parents TLD test (positive + negative)	✅ Fixed
tray.rs emit error logging	✅ Fixed
parse_blocklist_domains no-op + analysis comment	✅ Documented
bannerText CSS regression	❌ STILL BROKEN
validate_whitelist_domain tests	❌ STILL NONE
Carried v2 nit: confirmDialog button-disable	(open)
Carried v2 nit: onPointerDown repetition	(open)

Recommendation: ✅ Approve once the two v2 blockers are addressed — the CSS fix is a 5-line CSS change and the validation tests are ~40 lines of #[test] blocks. Both are mechanical and don't require design discussion.

After those land, this is a solid merge. The cold_start_hot_reload_blocks_first_query integration test is genuinely high-quality work — it caught the cold-start as a tested invariant rather than a documented one. ★

---

Verified during this re-review (line numbers from current feat/ad-block-dns)

• state/mod.rs:62 — dns_server: Arc<Mutex<Option<mhost_dns::DnsServer>>> ✓
• state/mod.rs:188-219 — cold-start wrapped in spawn_blocking ✓
• commands/adblock.rs:48-53 — MAX_URL_LEN=2048, MAX_DOMAIN_LEN=253 ✓
• commands/adblock.rs:286-291 — length guard inside validate_whitelist_domain ✓
• commands/adblock.rs:612-617 — MAX_URL_LEN guard inside add_ad_block_source_impl ✓
• commands/adblock.rs:1047-1174 — cold_start_hot_reload_blocks_first_query integration test ✓
• commands/adblock.rs test count: 5 #[test] + 2 #[tokio::test] — none reference validate_whitelist_domain ❌
• matcher.rs:78-103 — walk_parents_registered_tld_matches_every_subdomain ✓
• tray.rs:226-232 — if let Err(e) = ... { tracing::warn! } ✓
• App.test.tsx:60-67, 117-118 — count assertion with >= ✓
• AdBlock.tsx:152 — <div className={styles.bannerText}> — wrong layout ❌
• AdBlock.module.css:191-197 — .bannerText { flex: 1; display: flex; flex-direction: column; gap: 2px; } — wrong layout ❌

[flyhigher139]

PR #154 Re-review (after commit 410e2ee3)

Diff: +4449/-32 across 31 files (12 commits, +1 since v3)
Scope of fix commit: closed 1 P1 + 1 P2 + 1 P3 from round 3 — .bannerText CSS layout, validate_whitelist_domain test coverage, CSS trailing newline.

Headline: All three round-3 blockers closed correctly. The bannerText CSS now matches the original inline style; the 8 new tests cover all 7 rejection branches + happy path + the 253/254 boundary; the CSS file ends with a newline. Ready to merge.

---

Round-3 blockers — all closed ✓

[P1 → ✅] .bannerText CSS layout fixed

File: src/pages/AdBlock.module.css:194-199

.bannerText {
  display: flex;
  justify-content: space-between;
  align-items: center;
  gap: 12px;
}

Verified live via GitHub API (file size 4047 bytes; bannerText CSS now matches the original v1 inline style: display: flex; justify-content: space-between; align-items: center; gap: 12). The master switch toggle will now sit to the right of the "Enable Ad Block" label rather than stacking under the description. The misleading flex: 1 + flex-direction: column from round 2 are gone.

[P2 → ✅] validate_whitelist_domain now has 8 unit tests

File: src-tauri/src/commands/adblock.rs:997-1066

Live file confirms 13 #[test] + 2 #[tokio::test] (was 5 + 2 before this commit). All 7 rejection branches + happy path + the 253/254 boundary are covered:

Test	Branch
validate_whitelist_domain_happy_path_lowercases_and_trims	happy path (trim + lowercase)
validate_whitelist_domain_rejects_empty_or_whitespace_only	empty after trim
validate_whitelist_domain_rejects_whitespace_inside	internal whitespace
validate_whitelist_domain_rejects_path_separator	/ and \
validate_whitelist_domain_rejects_wildcard	*
validate_whitelist_domain_rejects_leading_dot	leading .
validate_whitelist_domain_rejects_unicode	non-[a-z0-9.-] (Unicode)
validate_whitelist_domain_rejects_oversize	MAX_DOMAIN_LEN boundary — 254 reject, 253 pass

Each test also asserts on the error message string (where useful), so a future refactor that changes "whitelist entry has invalid character" to "invalid character" gets caught by the contains checks. Solid.

The commit message claims 418 passed (was 410; +8 from validation tests) — matches the file diff (5 + 8 = 13 #[test]).

[P3 → ✅] AdBlock.module.css trailing newline

File: src/pages/AdBlock.module.css — last 30 bytes now : 12px;\n padding: 2px 6px;\n}\n (vs the previous }\ No newline at end of file). Trivial but consistent with the rest of the codebase now that App.tsx + AdBlock.tsx also have it.

---

CI status

PR checks: Detect changes ✓ (7s), Frontend Checks pending, Rust Checks pending. Locally the author reports all clean (cargo clippy ... -D warnings, cargo fmt --check, cargo test --workspace --lib 418 passed, pnpm test 255 passed). Should turn green on the next poll.

---

What's left (carried from earlier rounds, all optional)

[P3] remove_ad_block_whitelist silently no-ops on invalid input

Intentional. Acceptable.

[P3] confirmDialog doesn't disable Delete button while pending

Carried from v1. Acceptable; would need local useState<Set<string>> to fix.

[P3] onPointerDown={onPointerDown(() => {})} repeated 10+ times

Carried from v1. Cosmetic; wrapper component would clean up.

[P3] Frontend strings not i18n'd

Out of scope — rest of the app is also English-only.

[P3] pick_free_port in cold_start_hot_reload_blocks_first_query has a race

The helper does bind("127.0.0.1:0") then immediately drop(listener) to "release" the port. On most OSes the port is released on drop, leaving a small window for another process to grab it before the test binds. The polling loop (server.is_running() + 10ms sleeps + 2s deadline) catches most races, but a cleaner approach would be to keep the listener socket open with SO_REUSEADDR and pass it through. Doesn't affect correctness in practice. Out of scope for this merge.

---

Final recommendation

✅ Approve and merge.

All three round-3 blockers closed. The PR delivers:

• Core feature: Arc::swap atomic rule publication (issue #132), cold-start recovery (PR #131 P1-1), persist-on-fetch-failure (P1-2), corruption-backup-with-collision-counter (finding 0.2), shared reqwest client (1.8 + 1.9), bounded concurrent refresh (1.4), cancel-before-abort (issue #138)
• Cold-start hot-reload integration test (cold_start_hot_reload_blocks_first_query) — real UDP round-trip asserting 0.0.0.0 — locks in the headline fix as a tested invariant rather than a documented one
• Validation coverage: 8 unit tests for validate_whitelist_domain covering every rejection branch + boundary
• Storage safety: atomic_write throughout, two-stage size enforcement, microsecond-precision backup-collision counter
• Clean migration cut: confirmed exclusions of DNS hardening series (#140, #142, #146, #148, #149, #152) — grep-verified no leaked fields

All round-1, round-2, round-3 findings are addressed. The remaining items are P3 nitpick candidates for follow-up issues, not merge blockers.

---

Verified during this re-review (live feat/ad-block-dns)

• AdBlock.module.css:194-199 — bannerText CSS = display: flex; justify-content: space-between; align-items: center; gap: 12px; ✓
• AdBlock.module.css last 30 bytes = ': 12px;\n padding: 2px 6px;\n}\n' ✓ (newline present)
• commands/adblock.rs — 13 #[test] + 2 #[tokio::test] ✓
• commands/adblock.rs:1004-1066 — 8 validate_whitelist_domain_* test functions present ✓
• Test count claim matches: 410 + 8 = 418 ✓
• AdBlock.tsx:152 — still uses <div className={styles.bannerText}> (unchanged; layout now correct via CSS) ✓