feat(gmail): add +read helper for extracting message body as plain text#465
feat(gmail): add +read helper for extracting message body as plain text#465anshul-garg27 wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 93d39dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new gws gmail +read helper to fetch and display Gmail messages, which is a useful addition. The implementation is mostly solid, with good argument parsing and tests. However, there is significant code duplication for fetching the message data, which has already been implemented in the fetch_message_metadata helper function. Reusing this existing function will not only simplify the code but also add resilience by incorporating the existing retry logic that is currently missing in the new implementation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a useful gws gmail +read helper for fetching and displaying Gmail messages. The implementation is solid, but I've identified an opportunity to improve performance by reducing string allocations when constructing the output. My suggestion refactors the logic to move data instead of cloning it, which is more idiomatic and efficient in Rust.
|
Ready for review — all tests passing, Gemini feedback addressed. @jpoehnelt |
|
/gemini review |
Closes googleworkspace#438. Add `gws gmail +read` helper that fetches a Gmail message and extracts its body as plain text (or HTML with --html). Supports: - --message-id: Gmail message ID to read (required) - --html: return HTML body instead of plain text - --body-only: print just the body without headers/metadata - --format: output format (json, table, yaml, csv) Reuses the existing parse_original_message() infrastructure from reply/forward to extract headers and body content, including base64-decoded MIME parts.
…ogic Address Gemini review: replace manual HTTP fetch + parse with the existing fetch_message_metadata() helper which also uses send_with_retry for transient error resilience.
c0e85e1 to
93d39dd
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new gws gmail +read helper, which is a great addition for fetching and inspecting Gmail messages. The implementation is clean and reuses existing helpers effectively. I've found one high-severity issue related to input handling that could lead to unexpected API failures. My feedback includes a code suggestion to address this.
| let message_id = matches.get_one::<String>("message-id").unwrap().to_string(); | ||
|
|
||
| if message_id.trim().is_empty() { | ||
| return Err(GwsError::Validation( | ||
| "--message-id must not be empty".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The message-id is not trimmed before being used. If a user provides a message ID with leading or trailing whitespace (e.g., from a copy-paste), the validation will pass, but the API call will likely fail because the whitespace will be URL-encoded as part of the ID. The message-id should be trimmed before validation and use to prevent these unnecessary failures and improve usability.
| let message_id = matches.get_one::<String>("message-id").unwrap().to_string(); | |
| if message_id.trim().is_empty() { | |
| return Err(GwsError::Validation( | |
| "--message-id must not be empty".to_string(), | |
| )); | |
| } | |
| let message_id = matches.get_one::<String>("message-id").unwrap().trim().to_string(); | |
| if message_id.is_empty() { | |
| return Err(GwsError::Validation( | |
| "--message-id must not be empty".to_string(), | |
| )); | |
| } |
There was a problem hiding this comment.
good catch, added a .trim() on the message-id before validation
There was a problem hiding this comment.
Code Review
This pull request introduces a +read helper for Gmail, which is a great addition. The implementation is mostly solid, but I've identified a couple of areas for improvement. One is to make argument parsing for the output format more robust using clap's value_parser. The other is a refactoring opportunity in handle_read to improve efficiency by avoiding several unnecessary string allocations. These changes will make the new helper more robust and performant.
| Arg::new("format") | ||
| .long("format") | ||
| .help("Output format: json (default), table, yaml, csv") | ||
| .value_name("FORMAT"), |
There was a problem hiding this comment.
For better robustness, it's a good practice to use value_parser to validate the format argument against the allowed values. This ensures that clap will reject any invalid format string before it reaches your handler code, preventing potential panics or unexpected behavior from the OutputFormat::from_str call.
| Arg::new("format") | |
| .long("format") | |
| .help("Output format: json (default), table, yaml, csv") | |
| .value_name("FORMAT"), | |
| Arg::new("format") | |
| .long("format") | |
| .help("Output format: json (default), table, yaml, csv") | |
| .value_name("FORMAT") | |
| .value_parser(["json", "table", "yaml", "csv"]), |
There was a problem hiding this comment.
makes sense, added value_parser for the format arg so clap rejects bad values upfront
| let body = if config.html { | ||
| parsed | ||
| .body_html | ||
| .clone() | ||
| .unwrap_or_else(|| parsed.body_text.clone()) | ||
| } else { | ||
| parsed.body_text.clone() | ||
| }; | ||
|
|
||
| if config.body_only { | ||
| println!("{body}"); | ||
| } else { | ||
| let output = json!({ | ||
| "id": config.message_id, | ||
| "from": parsed.from, | ||
| "to": parsed.to, | ||
| "cc": parsed.cc, | ||
| "subject": parsed.subject, | ||
| "date": parsed.date, | ||
| "body": body, | ||
| }); | ||
| println!("{}", crate::formatter::format_value(&output, &fmt)); | ||
| } |
There was a problem hiding this comment.
This block can be refactored to be more efficient by avoiding several string clones. By using references (&str) for the body content and for the fields passed to the json! macro, you can prevent unnecessary memory allocations. This also makes the code more idiomatic.
let body_ref = if config.html {
parsed.body_html.as_deref().unwrap_or(&parsed.body_text)
} else {
&parsed.body_text
};
if config.body_only {
println!("{body_ref}");
} else {
let output = json!({
"id": &config.message_id,
"from": &parsed.from,
"to": &parsed.to,
"cc": &parsed.cc,
"subject": &parsed.subject,
"date": &parsed.date,
"body": body_ref,
});
println!("{}", crate::formatter::format_value(&output, &fmt));
}There was a problem hiding this comment.
fair point on the clones, cleaned it up to use references where possible
Encapsulates the new `gws gmail +read` subcommand (introduced in googleworkspace#465) as an agent skill. The skill documents how to extract decoded plain-text message bodies from Gmail without dealing with raw MIME payloads, and updates the gws-gmail skill to list +read in its helper commands table. Closes googleworkspace#438 https://claude.ai/code/session_01PkDwmaayDdUhCDgurYesXC
Closes #438.
Summary
Add
gws gmail +readhelper that fetches a Gmail message and outputs its headers and body as structured data or plain text.Usage
Changes
src/helpers/gmail/read.rs—handle_read()fetches message via Gmail API, reusesparse_original_message()for header/body extractionsrc/helpers/gmail/mod.rs— register+readsubcommand and handlerTest plan
cargo test— 588 passed, 0 failedcargo clippy -- -D warnings— cleangws gmail +read --message-id <id>→ verify outputGenerated with Claude Code