perf(tui): lower the settings filter once per pass, not once per row (#6213 T6)

[Hmbown]

Summary

Part of #6213 (item T6).

No-Issue: partial work on #6213 — the filter-term hoist only; T4, T5 and the per-row half of T6 remain, so that issue must stay open.

What changed

row_matches_filter lowercased and split self.filter on every row, and the settings view calls it twice per interaction — once from visible_items, once from matching_row_indices. For a filter like auto approve, that is the same trim().to_lowercase() and the same split_whitespace() repeated 2 × rows times per keystroke.

The term list moves into filter_terms(), computed once per pass and passed in as &[String]. Empty terms still mean "everything matches", and the matching expression is untouched, so what the filter accepts is identical.

What deliberately did not change

The larger half of T6 — lowercasing each row's own label, value, scope and hint (about 10 allocations per row per pass) — is not here. Caching those needs the row_display_value dependency settled first: unlike the rest, it is not a pure function of the row, so a cached blob keyed on the row alone could go stale on screen. I would rather leave that visible than ship a cache with an unsound invalidation rule.

Verification

cargo check -p codewhale-tui --all-targets --all-features --locked   (clean)
test result: ok. 329 passed; 0 failed; 0 ignored; 0 measured; 12479 filtered out
    (tui::views::)

Local only, macOS/aarch64. No hosted CI claim; no benchmark, so the saving is argued from the removed work rather than measured.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

Claude finished @Hmbown's task in 1m 54s —— View job

---

Review checklist

• Gather context (PR body, diff, surrounding code)
• Verify semantic equivalence of the old vs. new filter logic
• Check all call sites of row_matches_filter were updated
• Check for reuse/duplication concerns
• Post review

Review: perf(tui): lower the settings filter once per pass, not once per row (#6213 T6)

This is a clean, correctly-scoped perf fix. I diffed crates/tui/src/tui/views/mod.rs between origin/main and HEAD and traced both call sites.

Correctness — verified equivalent.
The empty-filter short-circuit moved from filter.trim().to_lowercase().is_empty() (old) to terms.is_empty() (new, where terms comes from split_whitespace() on that same trimmed/lowercased string). These are equivalent: trim() already strips leading/trailing whitespace, so a non-empty trimmed string always yields ≥1 term from split_whitespace(), and an empty/all-whitespace string always yields zero terms. No behavior change for the "everything matches" case.

Both call sites (matching_row_indices at line 2761 and visible_items at line 2774) were updated consistently — each hoists let terms = self.filter_terms(); once above its row loop and threads &terms into row_matches_filter. No stale call site left calling the old single-arg signature.

Scope — matches what the PR claims.
filter_terms() is still computed twice per interaction (once per caller), which the PR description explicitly says is intentional/out of scope for this slice — only the once-per-row repetition is eliminated. The larger T6 half (per-row label/value/scope/hint lowercasing, ~10 allocations/row) is correctly left alone, and the PR gives a sound reason (unresolved row_display_value staleness/cache-invalidation concern) rather than silently doing a partial job.

Minor, non-blocking observations:

• filter_terms() returns Vec<String>, allocating one String per term. This could be Vec<&str> borrowing from a single locally-owned lowercased String to shave a few small allocations, but given filters are typically 1-3 words, this is negligible and the current version is simpler — not worth complicating for this slice.
• Doc comment says "computed once per interaction instead of once per row per pass" — slightly awkward phrasing but harmless.

No issues found with provider/model safety (not applicable — no provider/model code touched), security, or dead code. No shared split_whitespace/to_lowercase filter-term helper exists elsewhere in the crate that this duplicates.

Tests: No new tests added, but none are needed here — this is a pure refactor preserving observable filter behavior, verified against the existing 329-test tui::views:: suite per the PR description. Consistent with the repo's guidance to avoid tests-by-default for behavior-preserving perf work.

[codewhale-agent]

Codewhale review

Complete review coverage: 1/1 passes, 1 file patches, sha256:a2b103022e9acc5b7e52c6ab8107c5932eb17cc818cbe9e541ec78b15c3c4ac2.

Pass 1: The diff hoists the lowercased/split filter terms out of the per-row path into a new filter_terms() helper and threads &[String] through row_matches_filter, updating both call sites (matching_row_indices, visible_items). The matching expression itself is unchanged, and the empty-terms shortcut is behaviorally equivalent to the previous filter.trim().is_empty() early return.

Assessment

Pass 1: No defect could be established in the changed code. Behavioral equivalence holds: trim().to_lowercase() followed by split_whitespace() was already the exact sequence applied per row, and trim()/split_whitespace() use the same char::is_whitespace predicate, so a whitespace-only or empty filter still yields zero terms and terms.is_empty() still short-circuits to true exactly as filter.is_empty() did. The comparison chain (section.contains(term) || ...) is untouched; term is &String from terms.iter(), and str::contains accepts &String via the std Pattern impl, so the types are consistent with the previous &str terms. The borrows in both updated loops are all shared (&self, &terms), so there is no aliasing problem. Note the deliberate non-goal: row_display_value/label lowering is still per-row, and the PR itself says so; that is pre-existing work, not a regression here. Unverified context: the review only saw this one file's diff, and repository source excerpts were unavailable, so I could not enumerate every caller of the private row_matches_filter to confirm the two updated call sites are the only ones (a missed third caller would be a compile error). The PR's cargo check/test claims are self-reported and no build or tests were run for this review. Also pre-existing and unchanged by this diff: filtering = !self.filter.is_empty() treats a whitespace-only filter as active, disabling the category rail while showing all rows; that quirk predates this change and is worth a separate look, but it is not introduced here.

---

Advisory review by Codewhale (codewhale review --pr 6267 --post, head 75e032b61d93a391660c1006022cacf1e010aebc). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

[Hmbown]

Re-ran Test (ubuntu-latest); it passed. All five required checks are green, including Test (macos-latest).

The original failure was not the SHA-6380 env-var class the handoff note guessed. Under cargo-nextest each test runs in its own process, and this test spawns the real binary with env_clear() and a fresh TempDir HOME — an intra-process env race is structurally impossible. It was a 250 ms CLI_PERSIST_TIMEOUT race that lost the dry-run receipt under Ubuntu load, and it had happened once before on PR #6104 (c810bc0458, 2026-09-12) at the same :180.

Tracked as #6269, with the test-side half in #6270.