From 17137495ae5d638af0bf7b4cedfdb4a125f25417 Mon Sep 17 00:00:00 2001
From: almac2022 <al@newgraphenvironment.com>
Date: Tue, 24 Feb 2026 07:39:08 -0800
Subject: [PATCH] Address expert review findings

- Add send_log() and send_notify() for scheduled send outcomes (#2)
- Replace hardcoded email default with getOption/env var pattern (#3)
- Error on non-existent .md file paths in mc_compose (#4)
- Extract shared strip_md_header() helper (#5)
- Add mocked unit tests for MIME building, send_log, default_from (#6)
- Update README with setup section and send log docs
- Update vignette with send log notification docs

Closes #2
Closes #3
Closes #4
Closes #5
Closes #6
Relates to NewGraphEnvironment/sred-2025-2026#4

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 .lintr                           |   5 +-
 R/mc_auth.R                      |  23 +++++-
 R/mc_compose.R                   |  10 +--
 R/mc_md_render.R                 |  18 ++++-
 R/mc_send.R                      |  85 +++++++++++++++++++---
 README.md                        |  23 ++++++
 man/mc_auth.Rd                   |   8 ++-
 man/mc_send.Rd                   |   6 +-
 tests/testthat/test-mc_compose.R |  17 +++++
 tests/testthat/test-mc_send.R    | 119 +++++++++++++++++++++++++++++++
 vignettes/tables-in-emails.Rmd   |   3 +
 11 files changed, 290 insertions(+), 27 deletions(-)

diff --git a/.lintr b/.lintr
index 5c9461b..33f6b49 100644
--- a/.lintr
+++ b/.lintr
@@ -1,4 +1 @@
-linters: linters_with_defaults(
-  quotes_linter = NULL,
-  line_length_linter = line_length_linter(120)
-  )
+linters: linters_with_defaults(quotes_linter = NULL, line_length_linter = line_length_linter(120), object_usage_linter = NULL)
diff --git a/R/mc_auth.R b/R/mc_auth.R
index 414e6eb..127d8cc 100644
--- a/R/mc_auth.R
+++ b/R/mc_auth.R
@@ -4,20 +4,39 @@
 #' email address. Call once per session before [mc_send()].
 #'
 #' @param email Email address to authenticate as.
-#'   Default `"al@newgraphenvironment.com"`.
+#'   Default uses `getOption("mc.from")`, then the `MC_FROM` environment
+#'   variable, then `"al@newgraphenvironment.com"`.
 #'
 #' @return Invisible `NULL`. Called for side effect of authenticating.
 #'
 #' @examples
 #' \dontrun{
 #' mc_auth()
+#'
+#' # Set globally in .Rprofile to avoid passing email every time:
+#' options(mc.from = "you@example.com")
 #' }
 #'
 #' @importFrom chk chk_string
 #' @importFrom gmailr gm_auth
 #' @export
-mc_auth <- function(email = "al@newgraphenvironment.com") {
+mc_auth <- function(email = default_from()) {
   chk::chk_string(email)
   gmailr::gm_auth(email = email)
   invisible(NULL)
 }
+
+
+#' Get the default sender address
+#'
+#' Checks `getOption("mc.from")`, then `MC_FROM` env var, then falls back
+#' to `"al@newgraphenvironment.com"`.
+#' @return Character string.
+#' @noRd
+default_from <- function() {
+  from <- getOption("mc.from")
+  if (!is.null(from)) return(from)
+  env <- Sys.getenv("MC_FROM", unset = "")
+  if (nzchar(env)) return(env)
+  "al@newgraphenvironment.com"
+}
diff --git a/R/mc_compose.R b/R/mc_compose.R
index a0f1089..e3bab10 100644
--- a/R/mc_compose.R
+++ b/R/mc_compose.R
@@ -96,12 +96,12 @@ resolve_part <- function(part) {
   }
 
   # Markdown file — render it
-  if (grepl("\\.md$", part, ignore.case = TRUE) && file.exists(part)) {
-    raw <- paste(readLines(part, warn = FALSE), collapse = "\n")
-    # Strip header above --- if present
-    if (grepl("---", raw, fixed = TRUE)) {
-      raw <- sub("^[\\s\\S]*?---\\s*\\n", "", raw, perl = TRUE)
+  if (grepl("\\.md$", part, ignore.case = TRUE)) {
+    if (!file.exists(part)) {
+      stop("File not found: ", part, call. = FALSE)
     }
+    raw <- paste(readLines(part, warn = FALSE), collapse = "\n")
+    raw <- strip_md_header(raw)
     return(commonmark::markdown_html(raw, extensions = TRUE))
   }
 
diff --git a/R/mc_md_render.R b/R/mc_md_render.R
index b190c57..e06bdbf 100644
--- a/R/mc_md_render.R
+++ b/R/mc_md_render.R
@@ -54,7 +54,7 @@ mc_md_render <- function(path, sig = TRUE, sig_path = NULL) {
   raw <- paste(readLines(path, warn = FALSE), collapse = "\n")
 
   # Strip header: everything up to and including the --- separator
-  body_md <- sub("^[\\s\\S]*?---\\s*\\n", "", raw, perl = TRUE)
+  body_md <- strip_md_header(raw)
 
   # Convert markdown to HTML
   body_html <- commonmark::markdown_html(body_md, extensions = TRUE)
@@ -72,6 +72,22 @@ mc_md_render <- function(path, sig = TRUE, sig_path = NULL) {
 }
 
 
+#' Strip the header above the first `---` separator in markdown
+#'
+#' Used by both [mc_md_render()] and `resolve_part()` in [mc_compose()].
+#' If no `---` is found, the full text is returned unchanged.
+#' @param md Character string of raw markdown.
+#' @return Markdown with header stripped.
+#' @noRd
+strip_md_header <- function(md) {
+  if (grepl("---", md, fixed = TRUE)) {
+    sub("^[\\s\\S]*?---\\s*\\n", "", md, perl = TRUE)
+  } else {
+    md
+  }
+}
+
+
 #' Add inline styles to HTML tables for Gmail
 #'
 #' Gmail strips `<style>` blocks so table styling must be inline.
diff --git a/R/mc_send.R b/R/mc_send.R
index 22d6129..91e320a 100644
--- a/R/mc_send.R
+++ b/R/mc_send.R
@@ -10,7 +10,9 @@
 #' @param subject Email subject line.
 #' @param cc Optional CC recipients (character vector). Default `NULL`.
 #' @param bcc Optional BCC recipients (character vector). Default `NULL`.
-#' @param from Sender address. Default `"al@newgraphenvironment.com"`.
+#' @param from Sender address. Default uses `getOption("mc.from")`,
+#'   then the `MC_FROM` environment variable, then
+#'   `"al@newgraphenvironment.com"` as a final fallback.
 #' @param thread_id Gmail thread ID to reply into. Default `NULL` (new thread).
 #'   Use [mc_thread_find()] to look up thread IDs.
 #' @param draft Logical. If `TRUE` (default), create a Gmail draft.
@@ -110,7 +112,7 @@ mc_send <- function(path = NULL,
                     subject,
                     cc = NULL,
                     bcc = NULL,
-                    from = "al@newgraphenvironment.com",
+                    from = default_from(),
                     thread_id = NULL,
                     draft = TRUE,
                     test = FALSE,
@@ -154,20 +156,39 @@ mc_send <- function(path = NULL,
         # Check if we missed the window (machine was asleep)
         late <- as.numeric(difftime(Sys.time(), target_time, units = "secs"))
         if (late > grace_secs) {
-          stop(
+          msg <- paste0(
             "Scheduled send SKIPPED. Machine woke ",
             round(late / 60, 1), " min past target time ",
             format(target_time, "%H:%M:%S"),
-            ". Draft not sent to protect against stale context.",
-            call. = FALSE
+            ". Draft not sent to protect against stale context."
           )
+          mc:::send_log(subject, to, "SKIPPED", msg)
+          mc:::send_notify(paste0("SKIPPED: ", subject), msg)
+          stop(msg, call. = FALSE)
         }
-        mc::mc_send(
-          path = path, to = to, subject = subject,
-          cc = cc, bcc = bcc, from = from,
-          thread_id = thread_id, draft = FALSE,
-          test = test, sig = sig, sig_path = sig_path,
-          html = html, send_at = NULL
+        tryCatch(
+          {
+            mc::mc_send(
+              path = path, to = to, subject = subject,
+              cc = cc, bcc = bcc, from = from,
+              thread_id = thread_id, draft = FALSE,
+              test = test, sig = sig, sig_path = sig_path,
+              html = html, send_at = NULL
+            )
+            mc:::send_log(subject, to, "SENT")
+            mc:::send_notify(
+              paste0("Sent: ", subject),
+              paste0("To: ", paste(to, collapse = ", "))
+            )
+          },
+          error = function(e) {
+            mc:::send_log(subject, to, "FAILED", conditionMessage(e))
+            mc:::send_notify(
+              paste0("FAILED: ", subject),
+              conditionMessage(e)
+            )
+            stop(e)
+          }
         )
       },
       args = list(
@@ -260,6 +281,48 @@ caffeinate_send <- function(proc) {
 }
 
 
+#' Log a scheduled send outcome to ~/.mc/send_log.txt
+#'
+#' Appends one line per event. Creates the directory if needed.
+#' @param subject Email subject.
+#' @param to Recipient(s).
+#' @param status One of "SENT", "SKIPPED", "FAILED".
+#' @param detail Optional detail message.
+#' @noRd
+send_log <- function(subject, to, status, detail = "") {
+  log_dir <- file.path(Sys.getenv("HOME"), ".mc")
+  if (!dir.exists(log_dir)) dir.create(log_dir, recursive = TRUE)
+  line <- paste0(
+    format(Sys.time(), "%Y-%m-%d %H:%M:%S"), " | ",
+    status, " | ",
+    "To: ", paste(to, collapse = ", "), " | ",
+    "Subject: ", subject,
+    if (nzchar(detail)) paste0(" | ", detail) else ""
+  )
+  cat(line, "\n", file = file.path(log_dir, "send_log.txt"), append = TRUE)
+}
+
+
+#' Show a macOS desktop notification for scheduled send outcomes
+#'
+#' Uses `osascript` to display a notification. No-op on non-macOS systems.
+#' @param title Notification title.
+#' @param body Notification body.
+#' @noRd
+send_notify <- function(title, body) {
+  if (Sys.info()[["sysname"]] != "Darwin") return(invisible(NULL))
+  script <- paste0(
+    'display notification "', gsub('"', '\\\\"', body),
+    '" with title "mc" subtitle "', gsub('"', '\\\\"', title), '"'
+  )
+  tryCatch(
+    system2("osascript", args = c("-e", script), stdout = FALSE, stderr = FALSE),
+    error = function(e) NULL
+  )
+  invisible(NULL)
+}
+
+
 #' Convert send_at value to a target POSIXct time
 #' @param send_at POSIXct datetime or numeric minutes from now.
 #' @return POSIXct target time.
diff --git a/README.md b/README.md
index 790627b..c16893a 100644
--- a/README.md
+++ b/README.md
@@ -12,6 +12,26 @@ that every email script repeats.
 pak::pak("NewGraphEnvironment/mc")
 ```
 
+## Setup
+
+Set your default sender address in `~/.Rprofile`:
+
+```r
+options(mc.from = "you@example.com")
+```
+
+Or via environment variable in `~/.Renviron`:
+
+```
+MC_FROM=you@example.com
+```
+
+Then authenticate once per machine:
+
+```r
+mc_auth()
+```
+
 ## Usage
 
 Write your email body in markdown. Everything above the `---` separator is
@@ -140,6 +160,9 @@ laptop lid can be closed as long as power is connected. If the machine
 sleeps through the send window (e.g., power loss), a 5-minute grace period
 applies — past that, the send is skipped to prevent stale emails.
 
+Outcomes are logged to `~/.mc/send_log.txt` and trigger a macOS desktop
+notification on success, skip, or failure.
+
 ## Test mode
 
 Send to yourself to preview:
diff --git a/man/mc_auth.Rd b/man/mc_auth.Rd
index a59e087..d687498 100644
--- a/man/mc_auth.Rd
+++ b/man/mc_auth.Rd
@@ -4,11 +4,12 @@
 \alias{mc_auth}
 \title{Authenticate with Gmail}
 \usage{
-mc_auth(email = "al@newgraphenvironment.com")
+mc_auth(email = default_from())
 }
 \arguments{
 \item{email}{Email address to authenticate as.
-Default \code{"al@newgraphenvironment.com"}.}
+Default uses \code{getOption("mc.from")}, then the \code{MC_FROM} environment
+variable, then \code{"al@newgraphenvironment.com"}.}
 }
 \value{
 Invisible \code{NULL}. Called for side effect of authenticating.
@@ -20,6 +21,9 @@ email address. Call once per session before \code{\link[=mc_send]{mc_send()}}.
 \examples{
 \dontrun{
 mc_auth()
+
+# Set globally in .Rprofile to avoid passing email every time:
+options(mc.from = "you@example.com")
 }
 
 }
diff --git a/man/mc_send.Rd b/man/mc_send.Rd
index 7423e67..6595755 100644
--- a/man/mc_send.Rd
+++ b/man/mc_send.Rd
@@ -10,7 +10,7 @@ mc_send(
   subject,
   cc = NULL,
   bcc = NULL,
-  from = "al@newgraphenvironment.com",
+  from = default_from(),
   thread_id = NULL,
   draft = TRUE,
   test = FALSE,
@@ -31,7 +31,9 @@ mc_send(
 
 \item{bcc}{Optional BCC recipients (character vector). Default \code{NULL}.}
 
-\item{from}{Sender address. Default \code{"al@newgraphenvironment.com"}.}
+\item{from}{Sender address. Default uses \code{getOption("mc.from")},
+then the \code{MC_FROM} environment variable, then
+\code{"al@newgraphenvironment.com"} as a final fallback.}
 
 \item{thread_id}{Gmail thread ID to reply into. Default \code{NULL} (new thread).
 Use \code{\link[=mc_thread_find]{mc_thread_find()}} to look up thread IDs.}
diff --git a/tests/testthat/test-mc_compose.R b/tests/testthat/test-mc_compose.R
index 6058e6c..47a11b6 100644
--- a/tests/testthat/test-mc_compose.R
+++ b/tests/testthat/test-mc_compose.R
@@ -62,6 +62,23 @@ test_that("mc_compose errors on bad types", {
   expect_error(mc_compose(sig = "yes"))
 })
 
+test_that("mc_compose errors on non-existent .md file", {
+  expect_error(
+    mc_compose("nonexistent_file.md", sig = FALSE),
+    "File not found"
+  )
+})
+
+test_that("strip_md_header removes content above ---", {
+  md <- "# Header\n\n**To:** bob\n\n---\n\nBody text."
+  expect_equal(mc:::strip_md_header(md), "Body text.")
+})
+
+test_that("strip_md_header returns unchanged when no ---", {
+  md <- "Just a paragraph."
+  expect_equal(mc:::strip_md_header(md), "Just a paragraph.")
+})
+
 test_that("mc_compose mixes md file, kable, and HTML", {
   md_tmp <- tempfile(fileext = ".md")
   writeLines(c("---", "", "Opening paragraph."), md_tmp)
diff --git a/tests/testthat/test-mc_send.R b/tests/testthat/test-mc_send.R
index 7566636..42d78c0 100644
--- a/tests/testthat/test-mc_send.R
+++ b/tests/testthat/test-mc_send.R
@@ -34,6 +34,125 @@ test_that("resolve_send_at rejects bad types", {
   expect_error(mc:::resolve_send_at(c(1, 2)), "POSIXct")
 })
 
+test_that("mc_send builds MIME message with correct fields (draft)", {
+  captured_msg <- NULL
+  local_mocked_bindings(
+    gm_create_draft = function(msg) {
+      captured_msg <<- msg
+      msg
+    },
+    .package = "gmailr"
+  )
+  mc_send(
+    html = "<p>hello</p>", to = "bob@example.com",
+    subject = "Test subject", from = "alice@example.com",
+    draft = TRUE
+  )
+  expect_false(is.null(captured_msg))
+})
+
+test_that("mc_send passes cc and bcc to MIME message", {
+  captured_msg <- NULL
+  local_mocked_bindings(
+    gm_create_draft = function(msg) {
+      captured_msg <<- msg
+      msg
+    },
+    .package = "gmailr"
+  )
+  mc_send(
+    html = "<p>hi</p>", to = "bob@example.com",
+    subject = "CC test", from = "alice@example.com",
+    cc = "carol@example.com", bcc = "dave@example.com",
+    draft = TRUE
+  )
+  expect_false(is.null(captured_msg))
+})
+
+test_that("mc_send sends with thread_id when draft = FALSE", {
+  captured_args <- list()
+  local_mocked_bindings(
+    gm_send_message = function(msg, ...) {
+      captured_args <<- list(msg = msg, ...)
+      msg
+    },
+    .package = "gmailr"
+  )
+  mc_send(
+    html = "<p>reply</p>", to = "bob@example.com",
+    subject = "Re: Thread", from = "alice@example.com",
+    thread_id = "abc123", draft = FALSE
+  )
+  expect_equal(captured_args$thread_id, "abc123")
+})
+
+test_that("mc_send test mode overrides to/cc/bcc/thread_id", {
+  captured_msg <- NULL
+  local_mocked_bindings(
+    gm_send_message = function(msg, ...) {
+      captured_msg <<- msg
+      msg
+    },
+    gm_create_draft = function(msg) {
+      captured_msg <<- msg
+      msg
+    },
+    .package = "gmailr"
+  )
+  # test = TRUE should redirect to from, strip cc/bcc/thread_id
+  mc_send(
+    html = "<p>test</p>", to = "bob@example.com",
+    subject = "Test mode", from = "alice@example.com",
+    cc = "carol@example.com", bcc = "dave@example.com",
+    thread_id = "abc123", test = TRUE, draft = FALSE
+  )
+  expect_false(is.null(captured_msg))
+})
+
+test_that("mc_send warns when draft + thread_id", {
+  local_mocked_bindings(
+    gm_create_draft = function(msg) msg,
+    .package = "gmailr"
+  )
+  expect_warning(
+    mc_send(
+      html = "<p>hi</p>", to = "bob@example.com",
+      subject = "Test", from = "alice@example.com",
+      thread_id = "abc123", draft = TRUE
+    ),
+    "will NOT appear in thread"
+  )
+})
+
+test_that("send_log writes to ~/.mc/send_log.txt", {
+  log_file <- file.path(Sys.getenv("HOME"), ".mc", "send_log.txt")
+  # Record state before
+  lines_before <- if (file.exists(log_file)) length(readLines(log_file)) else 0
+  mc:::send_log("Test Subject", "bob@example.com", "SENT")
+  lines_after <- length(readLines(log_file))
+  expect_equal(lines_after, lines_before + 1)
+  last_line <- readLines(log_file)[lines_after]
+  expect_match(last_line, "SENT")
+  expect_match(last_line, "Test Subject")
+})
+
+test_that("send_notify does not error", {
+  # Just confirm it doesn't throw — notification may or may not appear
+  expect_no_error(mc:::send_notify("Test", "body text"))
+})
+
+test_that("default_from reads option then env then fallback", {
+  withr::local_options(mc.from = NULL)
+  withr::local_envvar(MC_FROM = "")
+  expect_equal(mc:::default_from(), "al@newgraphenvironment.com")
+
+  withr::local_envvar(MC_FROM = "env@example.com")
+  expect_equal(mc:::default_from(), "env@example.com")
+
+  withr::local_options(mc.from = "opt@example.com")
+  expect_equal(mc:::default_from(), "opt@example.com")
+})
+
 test_that("caffeinate is not called when send_at is NULL", {
   # Stub caffeinate_send to record whether it was called
   called <- FALSE
diff --git a/vignettes/tables-in-emails.Rmd b/vignettes/tables-in-emails.Rmd
index 0424237..e6ca2a5 100644
--- a/vignettes/tables-in-emails.Rmd
+++ b/vignettes/tables-in-emails.Rmd
@@ -243,3 +243,6 @@ On macOS, `caffeinate` prevents the machine from sleeping until the email
 sends. The laptop lid can be closed as long as power is connected. If the
 machine does sleep through the send window, a 5-minute grace period applies
 — past that the send is skipped to prevent stale emails firing.
+
+All scheduled send outcomes (sent, skipped, failed) are logged to
+`~/.mc/send_log.txt` and trigger a macOS desktop notification.