feat: add group filter option - #639
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a new --group-filter (-g) option to filter exported group conversations by their display names. The feature works independently or in conjunction with the existing --conversation-filter option, using set intersection when both are specified.
Key changes:
- New CLI option
--group-filterfor filtering group chats by name - Runtime logic to resolve group names to chat IDs and intersect with existing filters
- Comprehensive test coverage for various filter scenarios
- Documentation updates explaining the intersection behavior
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| imessage-exporter/src/main.rs | Adds call to resolve_filtered_groups() after handle resolution |
| imessage-exporter/src/app/options.rs | Defines new CLI option, integrates into validation logic, updates test data |
| imessage-exporter/src/app/runtime.rs | Implements group filter resolution logic, validation, and comprehensive tests |
| imessage-exporter/src/exporters/txt.rs | Fixes hardcoded home directory path in test to use environment variable |
| imessage-exporter/src/exporters/html.rs | Fixes hardcoded home directory path in test to use environment variable |
| imessage-exporter/README.md | Documents new option and intersection behavior with conversation filter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| None => { | ||
| self.options | ||
| .query_context | ||
| .set_selected_chat_ids(matched_chat_ids.clone()); |
There was a problem hiding this comment.
The .clone() call on matched_chat_ids is necessary here since the variable is moved into set_selected_chat_ids. However, you could avoid this clone by restructuring the logic. Instead of matching on a reference and then calling the method separately, you could consume matched_chat_ids directly in both branches. For example, use if let Some(existing) = self.options.query_context.selected_chat_ids.take() and then set the result in both branches.
There was a problem hiding this comment.
Copilot is right here, in the None branch this clones a value that is not used afterward; pass it by value instead.
| "Sticker from Sample Contact: {sticker_path} (App: Free People) from Sample Contact", | ||
| sticker_path = sticker_path |
There was a problem hiding this comment.
The sticker_path named parameter in the format! macro is redundant. The variable is already captured by name in the format string. Remove the explicit sticker_path = sticker_path parameter.
| "Sticker from Sample Contact: {sticker_path} (App: Free People) from Sample Contact", | |
| sticker_path = sticker_path | |
| "Sticker from Sample Contact: {sticker_path} (App: Free People) from Sample Contact" |
| "<img src=\"{sticker_path}\" loading=\"lazy\">\n<div class=\"sticker_name\">App: Free People</div> <div class=\"sticker_tapback\"> by Sample Contact</div>", | ||
| sticker_path = sticker_path |
There was a problem hiding this comment.
The sticker_path named parameter in the format! macro is redundant. The variable is already captured by name in the format string. Remove the explicit sticker_path = sticker_path parameter.
| "<img src=\"{sticker_path}\" loading=\"lazy\">\n<div class=\"sticker_name\">App: Free People</div> <div class=\"sticker_tapback\"> by Sample Contact</div>", | |
| sticker_path = sticker_path | |
| "<img src=\"{sticker_path}\" loading=\"lazy\">\n<div class=\"sticker_name\">App: Free People</div> <div class=\"sticker_tapback\"> by Sample Contact</div>" |
|
Ignore copilot for now; I haven not yet decided if I want to include this feature yet. |
|
@ReagentX any update on this yet? |
|
Not yet |
|
Thanks for the update. I haven't lost track of this one. |
ReagentX
left a comment
There was a problem hiding this comment.
The overall approach is right, and the tests cover the cases that matter, we just need a few refinements to land this.
| .chatrooms | ||
| .iter() | ||
| .filter_map(|(&chat_id, chat)| { | ||
| let chat_name = chat.name(); |
There was a problem hiding this comment.
Chat::name() is the wrong source here. It falls back to chat_identifier when there is no custom name:
imessage-exporter/imessage-database/src/tables/chat.rs
Lines 120 to 127 in 9423e61
This means --group-filter does not actually filter on group names. For a 1:1 conversation, chat_identifier is the phone number or email, so -g "5558675309" matches a DM. For a group with no custom name, the only thing to match against is the opaque chatNNN… identifier, which no user would type. In practice the flag currently matches "any chat whose identifier or name contains the substring," which is not the goal of these changes.
Match on display_name() instead: a present display name is, in practice, the definition of a chat the user named, which is what we want this flag to select.
imessage-exporter/imessage-database/src/tables/chat.rs
Lines 129 to 141 in 9423e61
| /// App configuration options | ||
| pub options: Options, | ||
| /// Tracks whether the group filter matched any chats | ||
| pub group_filter_matches: bool, |
There was a problem hiding this comment.
This field is transient validation state living on a long-lived struct. resolve_filtered_groups writes it, start reads it, and the two only line up because of an undocumented requirement that resolution runs first. That is a coupling waiting to break: reorder the calls and the guard silently stops working.
I would rather resolve_filtered_groups return its outcome as Result<(), RuntimeError> and let main.rs, where the filters are already resolved, surface the error. This field then field disappears, there is nothing to keep in sync, and the ordering requirement becomes a compile-time argument rather than a convention.
| app::{contacts::Name, export_type::ExportType, runtime::filename_tests::fake_chat}, | ||
| }; | ||
|
|
||
| /// Verify that when the group filter matches no chats, the conversation filter is still honored. |
There was a problem hiding this comment.
This comment does not match runtime behavior. When group_filter is set and group_filter_matches is false, start() returns InvalidOptions and the export aborts. The preserved selected_chat_ids is never used.
The test verifies internal state that the runtime immediately discards, which is the opposite of what happens. Either the export should proceed on the conversation filter in this case, or the test's premise should be corrected.
|
|
||
| if let Some(filters) = &self.options.group_filter && !self.group_filter_matches { | ||
| return Err(RuntimeError::InvalidOptions(format!( | ||
| "Selected group filter `{filters}` does not match any chats!" |
There was a problem hiding this comment.
This log fires in two distinct situations: the group matched nothing, and the group matched chats that do not intersect --conversation-filter. Those are different problems: the second one is not "no match", rather, both filters matched, they just do not overlap.
| .collect(); | ||
|
|
||
| if matched_chat_ids.is_empty() { | ||
| self.group_filter_matches = false; |
There was a problem hiding this comment.
Redundant as self.group_filter_matches is set to False at the top of the function.
| .collect::<BTreeSet<i32>>(); | ||
|
|
||
| if intersection.is_empty() { | ||
| self.group_filter_matches = false; |
There was a problem hiding this comment.
Redundant as self.group_filter_matches is set to False at the top of the function.
| None => { | ||
| self.options | ||
| .query_context | ||
| .set_selected_chat_ids(matched_chat_ids.clone()); |
There was a problem hiding this comment.
Copilot is right here, in the None branch this clones a value that is not used afterward; pass it by value instead.
|
|
||
| if matched_chat_ids.is_empty() { | ||
| self.group_filter_matches = false; | ||
| return; |
There was a problem hiding this comment.
This branch sets the flag and returns, which is the same shape as the no-overlap branch above it. If this function returns a Result (see the comment on group_filter_matches), both collapse into one error path.
| To provide multiple group names, use a comma-separated string | ||
| Example: `-g "Family Chat,Work Group"` | ||
|
|
||
| Note: When both `--conversation-filter` and `--group-filter` are specified, only conversations that match both filters (intersection) will be exported. |
There was a problem hiding this comment.
The note should be indented as it would be displayed in the shell when you actually run -h, or it should exist elsewhere. Notes do not belong in this section as it is the literal output of -h.
This is based on the discussion here: #638