Skip to content

Improve TUI search, deletion, and export workflows - #723

Open
salmonumbrella wants to merge 11 commits into
kenn-io:mainfrom
salmonumbrella:fix/issues-40-41-44-72-88
Open

Improve TUI search, deletion, and export workflows#723
salmonumbrella wants to merge 11 commits into
kenn-io:mainfrom
salmonumbrella:fix/issues-40-41-44-72-88

Conversation

@salmonumbrella

Copy link
Copy Markdown
Contributor

What changed

  • Added session-only inline search history in the TUI, including draft restore and a 100-query cap.
  • Split deletion staging so d uses the selection/current row and D resolves every message-list filter or search match in the background before confirmation.
  • Added [data].export_dir and routed TUI attachment ZIPs, downloads, and opened files through it.
  • Corrected export-eml usage and default-filename guidance.
  • Documented how incremental Gmail sync and expired-history recovery reconcile messages deleted at the source.

Why

Search-heavy cleanup should not require retyping queries or selecting one loaded page at a time. Attachment exports should also have one predictable destination, while the CLI and sync docs should describe the behavior users actually get.

The all-match deletion path keeps source boundaries intact, rejects bounded semantic results, paginates fast and deep searches, stays responsive during resolution, and always presents the final count before a batch is staged.

Closes #40
Closes #41
Closes #44
Closes #72
Closes #88

Usage

  • In inline search, press Up or Down to recall earlier queries and return to the draft you were typing.
  • In a message list, press d for the current selection or D for every filter/search match.
  • Set export_dir under [data] to choose where TUI attachment exports land. Relative paths resolve from the config directory; the default is <data_dir>/exports.
  • Run msgvault export-eml <message-id> [source-message-id], optionally with -o - for stdout.

@roborev-ci

roborev-ci Bot commented Aug 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (247e03c)

Verdict: Medium-severity issues found in the bulk-deletion path.

Medium

  • internal/tui/actions.go:218-254 — Bulk deletion with an active search converts all matching emails into deletion targets without restricting source types. Imported mbox/PST messages may produce manifests that delete-staged rejects as unsupported. Resolve matches through a source-aware deletion path or filter unsupported sources before creating the manifest.

  • internal/tui/actions.go:197-205 — Without an active search, bulk deletion does not apply all active list filters. In particular, WithAttachmentsOnly and empty-value bucket filters are ignored, potentially staging unrelated Gmail messages for deletion. Apply every active filter or resolve IDs through the normal filtered list path before creating the manifest.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 14m31s

@salmonumbrella
salmonumbrella force-pushed the fix/issues-40-41-44-72-88 branch from 247e03c to c8f1c6f Compare August 30, 2026 12:06
@roborev-ci

roborev-ci Bot commented Aug 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (c8f1c6f)

Verdict: High-risk scope and deletion-target issues remain in bulk deletion.

High

  • internal/tui/actions.go:232-234 — Bulk deletion can escape the active MessageFilter scope. Deep search and some local fast-search paths drop filters such as sender/recipient name, conversation ID, and empty-value targets, allowing D to stage messages outside the visible list. Resolve matches using the complete filter, or fail closed for unsupported filter combinations.

Medium

  • internal/tui/actions.go:269-296 — The daemon-backed engine lacks the optional message-ID resolver, so the fallback includes every Gmail result without checking MessageSummary.DeletedAt. Retained source-deleted messages may therefore enter the deletion manifest. Add a daemon-backed resolver or skip messages with non-nil DeletedAt.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 14m16s

@salmonumbrella
salmonumbrella force-pushed the fix/issues-40-41-44-72-88 branch from c8f1c6f to 4fa0b70 Compare August 30, 2026 12:18
@roborev-ci

roborev-ci Bot commented Aug 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (4fa0b70)

Verdict: Two medium-severity issues require attention; no critical or high-severity findings were reported.

Medium

  • internal/tui/actions.go:223-230 — Bulk deletion with a nonempty query passes unsupported filters to remote fast search, causing deletion staging to fail in filtered views. Use a compatible remote query path or apply client-side scope intersection.

  • internal/tui/actions.go:205-224 — Bulk deletion materializes all matching messages as full summaries, potentially causing excessive memory usage on large archives. Stream paginated IDs and process targets in bounded batches.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 8m51s

@salmonumbrella
salmonumbrella force-pushed the fix/issues-40-41-44-72-88 branch from 4fa0b70 to 9af74a9 Compare August 30, 2026 12:29
@roborev-ci

roborev-ci Bot commented Aug 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (9af74a9)

Verdict: One medium-severity issue requires attention; no critical or high-severity findings were reported.

Medium

  • Malformed search queries can widen bulk-deletion scopeinternal/tui/actions.go:205-210
    The bulk D workflow passes parsed search queries to the search function without checking parsed.Err(). Invalid filters such as before:not-a-date may leave constraints unset, causing an unfiltered deletion batch to include all live Gmail messages. Reject parser errors before staging IDs, and apply equivalent validation to the existing TUI search path where appropriate.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 19m37s

@salmonumbrella
salmonumbrella force-pushed the fix/issues-40-41-44-72-88 branch 2 times, most recently from 3026d35 to 3aa9b3c Compare August 30, 2026 13:24
@roborev-ci

roborev-ci Bot commented Aug 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (3aa9b3c)

Verdict: Medium-severity issue identified; no security vulnerabilities found.

Medium

  • internal/tui/actions.go:326-343 — Bulk deletion silently skips all messages when asynchronously loaded account metadata is unavailable or failed. In the normal daemon-backed TUI, pressing D may incorrectly report “no messages selected” despite matching messages existing. Gate deletion until account metadata loads successfully, resolve source metadata through the daemon, or return an explicit metadata error.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 10m3s

@salmonumbrella
salmonumbrella force-pushed the fix/issues-40-41-44-72-88 branch from 3aa9b3c to 5c791a3 Compare August 30, 2026 13:37
@roborev-ci

roborev-ci Bot commented Aug 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (5c791a3)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 17m50s

@wesm wesm self-assigned this Sep 1, 2026
@wesm

wesm commented Sep 1, 2026

Copy link
Copy Markdown
Member

reviewing

@wesm

wesm commented Sep 1, 2026

Copy link
Copy Markdown
Member

I pushed a follow-up in d117726c that fixes the review issues.

Here is what changed:

  • Invalid structured search text now stays as inline feedback while you type. It no longer opens a modal or consumes the next key. Semantic search now sends the natural-language query through unchanged.
  • Uppercase D now has a clear scope: in a message list it stages every match for the current filter or search; in an aggregate view it stages the current row. Lowercase d still stages the Space selection, or the current item when nothing is selected.
  • All-match resolution can now be canceled. Esc stops it, and q stops it before showing the quit prompt.
  • Filtered and searched deletion targets are resolved in one backend query, including daemon mode. This removes the live LIMIT/OFFSET paging that could skip or duplicate messages, and avoids the extra archive-wide passes.
  • All-match manifests now describe the actual filter and search scope. A stale Space selection can no longer change the description or audit fields.
  • A filter or search with no deletable matches now reports that directly instead of saying that no messages were selected.
  • The repeated loading-state check is now one helper.

The generated API client was updated for the daemon path. The full tagged Go test suite, go vet, lint, and the generated API check all pass.

@roborev-ci

roborev-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (d117726)

Verdict: 3 actionable findings remain—1 High and 2 Medium severity.

High

  • internal/tui/actions.go:275-276, internal/tui/keys.go:444-452 — All-match deletion using an empty sender/domain/etc. bucket drops EmptyValueTargets, causing unrelated active Gmail messages to be returned instead of only empty-bucket matches. Preserve empty aggregate predicates and handle them in the deletion-target resolver before staging the manifest.

Medium

  • internal/daemonclient/engine_adapter.go:83-95, internal/api/handlers.go:752-755 — Semantic search forwards raw operator-shaped queries, but the daemon still parses and rejects invalid structured operators before hybrid search. Queries such as before:not-a-date explain... fail remotely. Align the client and daemon contracts by validating/stripping operators client-side or allowing raw semantic text through the daemon parser.

  • internal/daemonclient/engine_adapter.go:83-95 — Forwarding both raw query operators and the view’s message_type can widen scope. For example, message_type:sms invoice in Email mode may send both SMS and email scopes, which the server can treat as an OR. Intersect query and view message-type constraints, returning no matches for disjoint scopes.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 10m1s

@wesm

wesm commented Sep 1, 2026

Copy link
Copy Markdown
Member

Fixed the three follow-up review findings:

  • Deleting an “(empty)” sender, recipient, display-name, domain, or label bucket now keeps that empty-bucket filter. It cannot fall back to every active Gmail message.
  • Semantic searches can include natural-language text that looks like a broken search operator. The client now sends that text in a form the daemon treats as text instead of rejecting it.
  • A message_type: in the query is now combined safely with the current TUI view. Matching types are sent once, and conflicting types return no results instead of widening the search.

I added regression tests for each case. The full test suite, go vet ./..., and the repository lint checks all pass.

@roborev-ci

roborev-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (a402d94)

Verdict: Two medium-severity issues require attention.

Medium

  • internal/tui/keys.go:547-553 — During bulk deletion, q and Ctrl+C are consumed for cancellation and do not reach normal quit handling, requiring users to press them again. Cancel the resolver, then forward these keys through the standard global-key handler.

  • internal/daemonclient/engine_adapter.go:103-108 — A semantic query containing only a matching message_type: operator becomes empty after moving the operator to MessageTypes. The HTTP endpoint rejects it with missing_query. Reject operator-only queries with inline feedback or preserve a non-empty query when sending the request.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 12m22s

@wesm

wesm commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reviewed both follow-up findings:

  • Fixed the operator-only semantic search case. A query such as message_type:email now shows “semantic search requires free text” beside the search box and keeps the current results. The daemon client also rejects it locally instead of sending an empty query.
  • I did not change the bulk-deletion quit keys because they already work on the first keypress. Both q and Ctrl+C cancel the resolver and then continue into the normal quit handler. The existing focused test confirms that q opens the quit confirmation immediately; Ctrl+C follows the same path and quits immediately.

I added focused coverage for the semantic case. The full test suite, go vet ./..., and the repository lint checks pass.

@roborev-ci

roborev-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (aadc5b5)

Verdict: One medium-severity issue requires correction; no material security issues were identified.

Medium

  • Empty queries are incorrectly treated as searchesinternal/tui/actions.go:275-297, internal/daemonclient/engine_adapter.go:1022-1026
    Semantically empty queries such as subject: or whitespace are canonicalized to an empty q but still sent with search_mode, causing daemon rejection instead of staging all current matches. Treat trimmed/canonical empty queries as no-search and omit q/search_mode.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 18m32s

@wesm

wesm commented Sep 1, 2026

Copy link
Copy Markdown
Member

Fixed this follow-up issue.

Whitespace and empty operators such as subject: now use the same filter-only deletion path as an empty search box. The deletion manifest records “all matches” without a search query or search mode, and daemon requests omit both q and search_mode. Malformed queries still return their validation error instead of falling back to a wider filter.

I added focused tests for both the TUI staging record and the exact daemon request. The full test suite, go vet ./..., and the repository lint checks pass.

@roborev-ci

roborev-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (66fa809)

Verdict: Two medium-severity issues require attention.

Medium

  • internal/query/sqlite.go:1507-1510 — Bulk deletion combines search predicates and the current filter with AND, unlike normal TUI searches where sender, recipient, and domain scopes use OR. This can cause deletion to affect only the intersection rather than the messages displayed. Reuse the normal query/filter composition semantics while preserving separate predicates for unsupported filter fields.

  • internal/tui/model.go:1635-1649 — Invalid or changed inline search input does not invalidate an in-flight search request. A stale response can still match searchRequestID and overwrite results after the UI reports the query as invalid. Invalidate relevant request IDs or verify responses against the current input before applying them.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 32m23s

@wesm

wesm commented Sep 1, 2026

Copy link
Copy Markdown
Member

Fixed both remaining review findings in 7383f92.

  • Bulk “D” deletion now uses the same sender, recipient, and domain matching rules as the normal TUI search. This keeps the staged set aligned with the messages on screen. Filters that search cannot represent directly—such as names, conversations, and empty buckets—still narrow the staged set.
  • Changing the inline search text, or switching to a mode where the text is invalid, now cancels the authority of older search and message-list requests. A late response can no longer replace the current results or clear the inline error.

I added regression coverage for fast and deep deletion searches, empty-bucket filters, changed input, and invalid input.

@roborev-ci

roborev-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown

roborev: Combined Review (7383f92)

Review Verdict: Changes require fixes before merge due to one high-severity and one medium-severity issue.

High

  • Aggregate deletion can stage messages outside the displayed scopeinternal/tui/keys.go:458-465
    Aggregate D ignores the active search query and top-level attachment-only filter, so users may delete unrelated messages in the same aggregate. Resolve aggregate deletion through the same complete, search-aware scope used by message-list bulk deletion, and add an integration test covering matching and non-matching messages.

Medium

  • All-match deletion provenance is lost in remote TUI deletionsinternal/tui/actions.go:180-196; internal/daemonclient/convert.go:127
    Manifest.RawFilter is recorded but omitted during daemon-client manifest conversion, causing remote deletions to lose their search/filter scope. Preserve RawFilter in the generated API request and add an HTTP round-trip test.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 16m17s

@wesm

wesm commented Sep 2, 2026

Copy link
Copy Markdown
Member

Fixed the two latest review findings in 8eef0b37.

  • Pressing D on a grouped row now stages only messages in the scope currently shown: the active search, attachment-only setting, account and drill-down filters, deleted-message setting, and the current row. The background lookup remains cancellable.
  • When the TUI connects through the daemon, the saved deletion record now keeps the exact search and filter details instead of dropping them.

I added a real SQLite regression test for the grouped-row scope and an HTTP round-trip test for the saved filter details. The full test suite, vet, lint, and generated-client consistency check pass.

@roborev-ci

roborev-ci Bot commented Sep 2, 2026

Copy link
Copy Markdown

roborev: Combined Review (8eef0b3)

Verdict: Two high-severity correctness issues remain in bulk deletion resolution.

High

  • internal/tui/actions.go:296-303, internal/query/sqlite.go:1278-1469: All-match deletion for empty sender, recipient, name, domain, or label buckets ignores EmptyValueTargets, potentially staging every active Gmail message. Apply empty-value predicates in the authoritative filter resolver and add an integration test.

  • internal/query/sqlite.go:1491-1514: Bulk search resolution does not preserve Fast-search filter semantics. Converting filter.Label into SQLite q.Labels changes exact-label matching into substring matching, so a Work view may stage Homework messages. Share predicate logic with displayed Fast results or retain view filters as independent predicates.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 15m5s

@wesm
wesm force-pushed the fix/issues-40-41-44-72-88 branch from 8eef0b3 to 9944314 Compare September 2, 2026 04:19
@wesm

wesm commented Sep 2, 2026

Copy link
Copy Markdown
Member

Fixed the two latest bulk-deletion findings in 9944314f.

  • Choosing an empty sender, recipient, name, domain, or label group now keeps that empty-group condition during deletion staging. It no longer falls back to every active Gmail message.
  • A label selected from the current view is now matched exactly. For example, a Work view will not include Homework. A label typed into search still supports partial matching as before.

I added real SQLite coverage for every empty group and for the Work/Homework case. I also squashed the PR to one commit, rebased it onto current main, and preserved the contributor as the commit author. The full test suite, lint, vet, and generated-client checks pass.

@roborev-ci

roborev-ci Bot commented Sep 2, 2026

Copy link
Copy Markdown

roborev: Combined Review (9944314)

Verdict: Medium-severity correctness and safety issues remain in search-aware bulk deletion and aggregate-view handling.

Medium

  • internal/query/sqlite.go:1480-1526 — Bulk deletion resolution uses SQLite predicates that differ from DuckDB’s visible search semantics, including recipient (to vs. to/cc/bcc), label, and domain matching. This can omit messages visible in the TUI. Share predicate logic or ensure backend parity with coverage.

  • internal/query/sqlite.go:1506 — An untrusted sender domain containing % or _ is inserted into a SQL LIKE pattern without escaping, potentially resolving unrelated messages for deletion. Treat the domain as an exact residual predicate or escape metacharacters with an explicit ESCAPE clause.

  • internal/tui/keys.go:458-467 — Aggregate-view D lacks a deletionLoading guard. Users can navigate or leave the view while asynchronous resolution runs, allowing a stale deletion confirmation to appear; Esc does not cancel the lookup. Apply the message-list cancellation/blocking behavior to aggregate views.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 13m21s

@wesm

wesm commented Sep 2, 2026

Copy link
Copy Markdown
Member

Fixed and pushed in a1422af0.

What changed:

  • Fast search and all-match deletion now use the same view filters. Recipient filters include To, CC, and BCC. Labels and domains stay exact.
  • Percent and underscore characters in a domain are now treated as normal text, not wildcards. The local search backend also checks every sender on messages with more than one sender.
  • Aggregate D now keeps the current view in place while it resolves matches. Escape cancels the lookup, so an old confirmation cannot appear after navigation.

I added regression tests for these cases. The query and TUI test suites, lint, and vet pass.

@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (43ad020)

Verdict: One medium-severity issue requires attention; no high or critical findings were reported.

Medium

  • Stale Parquet cache can widen scoped deletions
    Location: internal/query/duckdb.go:2317
    Deletion paths may select message IDs from a stale Parquet cache, then hydrate them without reapplying the requested sender, recipient, label, attachment, domain, or other filters. This can cause messages no longer matching the displayed scope to be deleted after confirmation.
    Fix: Use SQLite’s authoritative filter-based deletion queries, or reapply the complete original filter when hydrating cached candidate IDs.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 21m14s

@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (3e0bf4e)

High: Aggregate-row bulk deletion can target messages outside the displayed search scope.

  • Location: internal/query/duckdb.go:1548-1557, internal/query/duckdb.go:2377-2389; related SQLite logic at internal/query/sqlite.go:1598
  • Problem: DuckDB aggregate searches omit message bodies, while deletion resolution uses SQLite’s body-aware search. A body-only match can therefore be hidden from the displayed aggregate row but still staged—and potentially deleted—when pressing uppercase D.
  • Fix: Use the same aggregate-search predicate for rendering and deletion resolution, or route both operations through the same body-aware backend.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 24m42s

@wesm

wesm commented Sep 3, 2026

Copy link
Copy Markdown
Member

Fixed the aggregate deletion scope issue. When an aggregate view has an active search, the rows, header totals, and deletion lookup now use the same body-aware search path, so a message cannot be staged because of a body match that was missing from the displayed row. I also added a regression test for that case and regenerated the browser API types that caused the linked CI failure.

@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (52ab452)

Verdict: One medium-severity pagination bug found; no security issues reported.

Medium

  • internal/api/handlers.go:3824-3828 — Body-scoped deep search reports total_count = offset + len(messages) for empty pages. Requests beyond the end can therefore return the offset as the match count, causing incorrect totals and phantom pagination. Return the unknown-count sentinel (-1) when an exact count is unavailable, or perform an exact count query.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 37m34s

@roborev-ci

roborev-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

roborev: Combined Review (d4a1613)

Verdict: One medium-severity issue found; otherwise no concrete issues identified.

Medium

  • Aggregate key-only searches can return incomplete results and incorrect stats. Aggregate searches delegated to SQLite omit grouping-key predicates for labels, sender names, and recipient names, while the stats path ignores GroupBy. Terms matching only an aggregate key may disappear, and totals may be wrong.
    Locations: internal/query/duckdb.go:1513-1514, internal/query/sqlite.go:728-735
    Fix: Preserve grouping-key predicates when delegating aggregate searches and stats to SQLite.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 34m50s

@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (166e569)

Verdict: No Medium, High, or Critical findings; the code is clean at the required severity threshold.


Reviewers: 2 done | Synthesis: codex, 6s | Total: 53m24s

@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (e5fb202)

Verdict: One medium-severity compatibility issue remains.

Medium

  • internal/daemonclient/engine_adapter.go:1071-1090 — Fast TUI searches send sender_name, recipient_name, and empty_targets, but SearchFastWithStats does not check API schema compatibility. Supported daemons as old as 2.14.0 reject these filters, causing remote searches to fail. Require schema 2.16.0 for these filters or provide a legacy-compatible fallback.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 21m53s

@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (89970a8)

Verdict: No Medium, High, or Critical findings identified.

The implementation preserves authentication, source scoping, search/deletion filters, and daemon schema checks.


Reviewers: 2 done | Synthesis: codex, 6s | Total: 49m55s

Add session search history, safe all-match deletion staging, and configurable
attachment exports. Correct export-eml guidance and document Gmail deletion
reconciliation.

Follow-up fixes:

- Keep aggregate statistics within the displayed search scope.
- Preserve narrow deletion and search filters across local and remote paths.
- Resolve deletion targets from authoritative live state.
- Align aggregate deletion with body-aware search results.
- Avoid fabricated totals on empty body-search pages.
- Preserve aggregate-key-only matches and their statistics.
- Follow the repository's testify helper policy in new tests.
- Gate complete fast-search filters on daemon API schema 2.16.

Generated with Codex
Co-authored-by: Wes McKinney <wesmckinn+git@gmail.com>
Co-authored-by: Codex <noreply@openai.com>
@wesm
wesm force-pushed the fix/issues-40-41-44-72-88 branch from 89970a8 to 8ec3cba Compare September 4, 2026 03:05
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (8ec3cba)

Verdict: One medium-severity consistency issue remains; no concrete security vulnerability was identified.

Medium

  • internal/query/sqlite.go:824-840 (buildAggregateStatsSearchParts): Repeated label: filters are treated as an OR for grouped aggregate rows but remain separate message-level predicates for aggregate statistics, requiring all labels. This can make header totals disagree with displayed rows. Mirror the label-specific handling from buildAggregateSearchParts, applying the OR predicate to the grouped label and excluding those labels from generic search processing.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 47m32s

Repeated label filters select matching aggregate rows as alternatives. The
statistics path instead required every label on each message, so header totals
could disagree with the displayed rows.

Apply the grouped-label predicate before generic search processing so both
paths count the same message population.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (3d579a3)

Verdict: One medium-severity consistency issue remains; no security regressions were identified.

Medium

  • internal/query/duckdb.go:995-1025 — Parquet-only aggregate statistics use a narrower free-text predicate than displayed aggregate rows. Recipient/name and label statistics check only the grouping key, so searches matching a subject, snippet, or sender may display rows while header totals report zero or incorrect counts.
    • Fix: Share the complete aggregate-search predicate between row aggregation and statistics, with regression coverage for subject/sender matches in recipient and label views.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 23m5s

Recipient and label aggregate rows search both message metadata and the
grouping key. Their header statistics searched only the grouping key, so a
subject or sender match could appear with a zero total.

Use the same message-level text scope for rows and statistics. Keep the
recipient or label key as an additional match for its view.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (8c3a496)

Verdict: One medium-severity issue found; no high or critical issues.

Medium

  • internal/query/duckdb.go:1013-1032 — Cache-only DuckDB aggregate statistics do not use the same grouping-key matching as displayed rows for sender/recipient names. Phone-only values and per-message display names can cause aggregate rows to match while header statistics report zero or undercounted results. Share the aggregate key predicate with stats, including mr.display_name, phone, email, and participant display name, and add a cache-only regression test.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 24m53s

Name aggregate rows can match a per-message display name or a phone fallback.
Cache-only statistics omitted those fields, so the header could report zero
while matching sender or recipient rows were visible.

Use one field list for name-row filtering and statistics so their scopes stay
aligned.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (53728c5)

Verdict: One medium-severity correctness issue remains; no security issues were identified.

Medium

  • Aggregate label matching can produce inconsistent countsinternal/query/sqlite.go:831-855; internal/query/duckdb.go:1040-1059
    Aggregate statistics evaluate label: and free-text label matching in separate message-level EXISTS clauses, while displayed aggregate rows require both predicates on the same label row. A message labeled Work and Needle may therefore be counted for label:Work Needle even though no displayed label row matches both conditions. Build aggregate statistics from the same row-correlated predicate used for displayed aggregates and add a regression test.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 42m42s

wesm and others added 2 commits September 4, 2026 08:57
Label aggregate rows require structured and free-text label matches on the
same label. Statistics matched them on separate labels, so header totals could
include messages with no visible matching row.

Keep both predicates in one label-row scope across SQLite and cached analytics.
Also align the affected CI expectation and documentation route with current
search behavior and site layout.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
Current main and this branch both add API schema 2.16 features. Preserve the
complete search contracts and the new asynchronous import contracts so the PR
can build and test against the current base branch.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (f952015)

Verdict: One high-severity and one medium-severity issue require attention.

High

  • docs/vercel-build.sh:25: The recursive copy publishes all files under website/assets, fonts, scripts, and styles. The filename blocklist may miss provider-generated credential files such as acme_secret.json or project-12345.json, potentially exposing OAuth or service-account secrets through the Vercel site. Build from a clean tracked-file allowlist or reject all non-approved files/extensions.

Medium

  • internal/tui/actions.go:56-74,224-242,500-525: All-match deletion can use an exact MessageFilter.ListID, but the list ID is omitted from serialized RawFilter.match_filter and Filters.ListIDs in the manifest. The saved manifest therefore cannot accurately describe or reproduce the scope that produced its message IDs. Serialize ListID, populate Filters.ListIDs, and add a regression test for all-match staging from a list-scoped view.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 25m59s

All-match staging already uses an exact List-ID filter to select messages, but
the manifest dropped that filter. The saved audit record could not explain or
reproduce the scope that produced its message IDs.

Preserve the list ID in both the structured filters and raw match provenance.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (0413b6e)

Verdict: Two medium-severity issues require attention before merge.

Medium

  • Time-based aggregate deletion may resolve no targets when granularity is omitted
    internal/query/sqlite.go:1735
    The resolver uses filter.TimeRange.Granularity directly, defaulting omitted values to year, while displayed aggregates default to month. A visible row such as 2024-01 may therefore produce no deletion targets. Carry the effective aggregate granularity into the resolver, defaulting omitted values to month while preserving explicit year/day selections.

  • Aggregate bulk deletion can act on stale rows after a scope change
    internal/tui/keys.go:467-476
    The uppercase D handler remains actionable during asynchronous reloads and combines the new scope with an old aggregate row. This can stage and delete messages outside the visible row. Disable deletion while loading, invalidate rows on scope changes, or associate rows with a load/scope version and reject mismatches.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 30m31s

Aggregate deletion could use a yearly predicate for a monthly key when the
request omitted its granularity. Uppercase D also remained active while a new
aggregate scope loaded, so it could combine fresh scope state with an old row.

Infer omitted granularity from the selected time key and wait for fresh
aggregate rows before resolving all matches.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (0c82747)

Verdict: One medium-severity correctness issue remains; otherwise, the changes are clean.

Medium

  • Phone-only recipients are omitted from recipient searches.
    Locations: internal/query/sqlite.go:2014-2032; internal/query/duckdb.go:3133-3162
    Recipient operators now match only email_address, excluding phone-only SMS/iMessage recipients from searches such as to:+15551234567, related counts, and deletion resolution. Match non-domain recipient values against both normalized email and phone fields, with regression coverage.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 59m30s

Recipient operators checked only email addresses. Messages addressed to a
phone number could disappear from search results, counts, and bulk deletion.

Match exact recipient values against both email and phone fields. Keep domain
suffix searches limited to email addresses.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 4, 2026

Copy link
Copy Markdown

roborev: Combined Review (4146994)

Verdict: Two medium-severity issues require attention.

Medium

  • internal/query/duckdb.go:1445-1463 — Structured recipient filters match only email_address, regressing fast searches for phone-only recipients. Include phone_number in recipient predicates, including combined recipient-name filters and deletion-target resolution.

  • internal/tui/model.go:1704-1715, internal/tui/keys.go:467-479 — Pressing Enter on an aggregate search before debounce completes updates searchQuery without reloading rows, allowing uppercase D to resolve a new query against stale results. Reload aggregates immediately on commit or disable deletion until rows match the committed query.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 33m38s

Phone-only recipients disappeared when a fast search used a view filter.
Aggregate search could also commit a new query before its matching rows loaded,
which made bulk deletion act on stale results.

Match recipient filters against email or phone data. Keep deletion unavailable
until rows for an unfinished aggregate search load.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (b0cf857)

Verdict: One medium-severity issue found; otherwise, the reviewed changes are clean.

Medium

  • internal/query/shared.go:85 — Copying an explicitly empty opts.SourceIDs slice with append([]int64(nil), ...) converts it to nil, losing the “match no sources” scope. Combined with opts.Filter, statistics may include all sources. Preserve the non-nil empty slice using make and copy.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 27m39s

An explicit empty source list means that no sources match. Combining it with a
complete filter erased that distinction, so statistics could include filtered
messages from any source.

Preserve the empty list when applying top-level statistics options to a filter.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Sep 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (6ae5f4d)

Verdict: One medium-severity consistency issue remains in SQLite recipient filtering.

Medium

  • SQLite structured recipient filters omit phone-only recipientsinternal/query/sqlite.go:462-479, internal/query/sqlite.go:1547-1565
    Predicates compare only email_address, causing SQLite searches, statistics, and deletion-target resolution to miss phone-only recipients supported by DuckDB and query operators. Match both email_address and phone_number, and add direct SQLite coverage.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 43m42s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants