fix(search): emails not excluded using nil thread id#2647
Conversation
…cipients for nil id for exclusion
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughEmail filtering logic migrates from using the Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/models_search/src/unified.rs`:
- Line 162: Remove the redundant call strip_nil_uuids(&mut
email_filters.recipients) from unified.rs: it’s no longer needed because the
inclusion/exclusion sentinel now lives in email_thread_ids and recipients holds
email addresses (see email_filters.email_thread_ids usage). Delete that call and
run a quick compile to remove any unused-import warnings for strip_nil_uuids (or
delete the helper import if it becomes unused); do not change any other
inclusion/exclusion logic that references email_thread_ids or the recipients
field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f95cfa6-fac7-4f1d-a41c-a819abac530e
📒 Files selected for processing (1)
rust/cloud-storage/models_search/src/unified.rs
| strip_nil_uuids(&mut document_filters.document_ids); | ||
| strip_nil_uuids(&mut chat_filters.chat_ids); | ||
| strip_nil_uuids(&mut email_filters.email_thread_ids); | ||
| strip_nil_uuids(&mut email_filters.recipients); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Any producer still writing NIL to email recipients (old exclusion convention)?
rg -nP --type=rust -C3 'email_filters\.recipients\s*='
# 2) Any code still reading recipients for a NIL-UUID inclusion/exclusion decision?
rg -nP --type=rust -C3 'email_filters\.recipients'
# 3) Tests around the new sentinel location and the changed conversion.
rg -nP --type=rust -C3 'email_thread_ids' -g '**/test*.rs' -g '**/*tests*.rs'
# 4) Spot-check callers of entity_filters_from_include and SearchEntityFilters::from for tests.
rg -nP --type=rust -C3 '\bentity_filters_from_include\s*\(|SearchEntityFilters::from\b|impl\s+From<EntityFilters>'Repository: macro-inc/macro
Length of output: 7567
🏁 Script executed:
# Find the EmailFilters structure definition and understand the recipients field
rg -nP --type=rust 'struct\s+EmailFilters|recipients\s*:' rust/cloud-storage/item_filters/src/lib.rs -A 15 | head -60
# Check for any NIL_UUID constants and their usage
rg -nP --type=rust 'NIL_UUID|nil.?uuid' rust/cloud-storage/ -i
# Look at the conversion from EntityFilters to SearchEntityFilters more carefully
sed -n '145,200p' rust/cloud-storage/models_search/src/unified.rsRepository: macro-inc/macro
Length of output: 6736
🏁 Script executed:
# Search for any test or code that assigns NIL_UUID to recipients
rg -nP --type=rust 'recipients.*NIL_UUID|NIL_UUID.*recipients' rust/cloud-storage/
# Check if any tests or code populates recipients with UUIDs (not email addresses)
rg -nP --type=rust 'recipients\s*[:=].*vec!\[|recipients.*push' rust/cloud-storage/ -A 2 -B 2Repository: macro-inc/macro
Length of output: 15770
🏁 Script executed:
# Get context around the test that uses recipients with nil
sed -n '4600,4680p' rust/cloud-storage/soup/src/outbound/pg_soup_repo/expanded/tests.rs
# Also search for the nil variable definition in that file
rg -nP 'let nil' rust/cloud-storage/soup/src/outbound/pg_soup_repo/expanded/tests.rs -B 2 -A 2Repository: macro-inc/macro
Length of output: 2910
🏁 Script executed:
# Check if the test that uses recipients with nil has been modified recently
cd rust/cloud-storage && git log -n 5 --oneline -- soup/src/outbound/pg_soup_repo/expanded/tests.rs
# Check the full test name and see if it's testing the sentinel behavior
grep -B 20 'recipients: vec!\[nil.clone()\]' rust/cloud-storage/soup/src/outbound/pg_soup_repo/expanded/tests.rs | grep -E '(#\[test|fn test_|async fn)'
# Check if this test was recently changed or if the change in the PR is supposed to update it
rg -nP 'test_all_filter_types_combined' rust/cloud-storage/ -A 60 | head -100Repository: macro-inc/macro
Length of output: 348
🏁 Script executed:
# Find the actual test file location
fd -type f -name "*.rs" -path "*soup*" -path "*tests*" | head -20
# Search for the test that uses recipients with nil
rg -nP 'test_all_filter_types_combined' --type=rust -A 60 | head -100Repository: macro-inc/macro
Length of output: 6598
🏁 Script executed:
# Check git history to see what the recent sentinel migration commit changed
git show 3bfdad63 --stat | head -30
# Look for any TODO or FIXME comments in the test file related to this
rg -nP '(TODO|FIXME|XXX).*recipients|recipients.*(TODO|FIXME|XXX)' rust/cloud-storage/soup/src/outbound/pg_soup_repo/expanded/tests.rs
# Check if there are recent changes or TODOs mentioning email_thread_ids and nil
rg -nP '(TODO|FIXME).*email_thread_ids|email_thread_ids.*(TODO|FIXME)' rust/cloud-storage/ --type=rustRepository: macro-inc/macro
Length of output: 1799
🏁 Script executed:
# Get the actual diff for files related to the sentinel migration
git show 3bfdad63 -- "*unified.rs" "*email*filters*" | head -200
# Check specifically what changed in the unified.rs file
git show 3bfdad63 -- rust/cloud-storage/models_search/src/unified.rs | head -150Repository: macro-inc/macro
Length of output: 16077
🏁 Script executed:
# Check what happens downstream with the recipients field
# Look for any conditional checks on recipients being empty or containing certain values
rg -nP 'recipients.*is_empty|email_filters\.recipients' rust/cloud-storage/ --type=rust -A 3 -B 3 | grep -E '(if|match|contains)' | head -30
# Check if the test has been recently modified or if there are related TODOs
git log -p --since="2026-03-01" -- rust/cloud-storage/soup/src/outbound/pg_soup_repo/expanded/tests.rs | grep -A 10 -B 10 'recipients.*nil' | head -50Repository: macro-inc/macro
Length of output: 2567
Remove the now-redundant NIL strip on recipients at line 162.
With the exclusion sentinel migrated to email_thread_ids, the strip_nil_uuids(&mut email_filters.recipients) call no longer serves any purpose. The inclusion/exclusion logic checks only email_thread_ids (line 155), not recipients. The recipients field holds email addresses per its documentation and should never contain a NIL UUID in normal flows. The strip is defensive code that obscures intent; if no legacy caller still populates recipients with NIL, this cleanup is safe to apply.
♻️ Proposed cleanup
strip_nil_uuids(&mut email_filters.email_thread_ids);
- strip_nil_uuids(&mut email_filters.recipients);
strip_nil_uuids(&mut channel_filters.channel_ids);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| strip_nil_uuids(&mut email_filters.recipients); | |
| strip_nil_uuids(&mut email_filters.email_thread_ids); | |
| strip_nil_uuids(&mut channel_filters.channel_ids); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/models_search/src/unified.rs` at line 162, Remove the
redundant call strip_nil_uuids(&mut email_filters.recipients) from unified.rs:
it’s no longer needed because the inclusion/exclusion sentinel now lives in
email_thread_ids and recipients holds email addresses (see
email_filters.email_thread_ids usage). Delete that call and run a quick compile
to remove any unused-import warnings for strip_nil_uuids (or delete the helper
import if it becomes unused); do not change any other inclusion/exclusion logic
that references email_thread_ids or the recipients field.
1 participant
This updates the search filter parsing to check
email_thread_idsrather thanrecipientsfor nil uuid instead to make it consistent with the checks for the other entity types