feat: Telescope Ctrl+r builds reply-all draft (#7)#18
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors contact resolution and calendar integration in nvim-mail to be asynchronous, extracting the resolver logic into a dedicated module and updating calendar events to use the actual event date. Feedback focuses on enhancing robustness against missing external commands (like khard, notmuch, and ldap_owa_query) by wrapping vim.system calls in pcall to prevent unhandled errors from stalling callbacks. Additionally, improvements are suggested to validate decoded JSON types, tighten the pattern matching for quoted signatures to avoid false positives, and prevent empty queries from whitespace-only header entries.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| vim.system(khard_cmd, { text = true }, function(result) | ||
| if result.code ~= 0 then | ||
| khard_results = {} | ||
| else | ||
| khard_results = M.parse_khard_output(result.stdout) | ||
| end | ||
| merge_and_deliver() | ||
| end) | ||
|
|
||
| -- Launch notmuch async (if enabled) | ||
| if notmuch_cmd then | ||
| vim.system(notmuch_cmd, { text = true }, function(result) | ||
| if result.code ~= 0 then | ||
| notmuch_results = {} | ||
| else | ||
| notmuch_results = M.parse_notmuch_output(result.stdout) | ||
| end | ||
| merge_and_deliver() | ||
| end) |
There was a problem hiding this comment.
If khard or notmuch is not installed or fails to run, vim.system will throw an unhandled error, which prevents merge_and_deliver from being called and leaves the completion callback pending indefinitely. Wrapping both async calls in pcall and falling back to empty results on failure ensures the completion callback is always invoked.
-- Launch khard async
local khard_ok = pcall(vim.system, khard_cmd, { text = true }, function(result)
if result.code ~= 0 then
khard_results = {}
else
khard_results = M.parse_khard_output(result.stdout)
end
merge_and_deliver()
end)
if not khard_ok then
khard_results = {}
merge_and_deliver()
end
-- Launch notmuch async (if enabled)
if notmuch_cmd then
local nm_ok = pcall(vim.system, notmuch_cmd, { text = true }, function(result)
if result.code ~= 0 then
notmuch_results = {}
else
notmuch_results = M.parse_notmuch_output(result.stdout)
end
merge_and_deliver()
end)
if not nm_ok then
notmuch_results = {}
merge_and_deliver()
end
end
| vim.system(cmd, { text = true }, function(result) | ||
| tried = tried + 1 | ||
| if found then return end | ||
| for _, line in ipairs(vim.split(result.stdout or '', '\n')) do | ||
| local dname, email = line:match('^(.-)%s*<([^>]+@' .. domain_pat .. ')>') | ||
| if email and dname and r.validate_notmuch_match(dname, name) then | ||
| done(email); return | ||
| end | ||
| end | ||
| if tried >= total then done(nil) end | ||
| end) |
There was a problem hiding this comment.
If notmuch is not installed or fails to execute, vim.system will throw an unhandled error, crashing the asynchronous resolution process and leaving the callback pending indefinitely. Wrapping vim.system in pcall and handling the failure by incrementing tried ensures the resolution chain completes gracefully.
local ok = pcall(vim.system, cmd, { text = true }, function(result)
tried = tried + 1
if found then return end
for _, line in ipairs(vim.split(result.stdout or '', '\n')) do
local dname, email = line:match('^(.-)%s*<([^>]+@' .. domain_pat .. ')>')
if email and dname and r.validate_notmuch_match(dname, name) then
done(email); return
end
end
if tried >= total then done(nil) end
end)
if not ok then
tried = tried + 1
if tried >= total then done(nil) end
end
| vim.system( | ||
| { 'ldap_owa_query', p[1] or norm2, p[2] or '', 'work' }, | ||
| { text = true, timeout = 10000 }, | ||
| function(result) | ||
| if result.code == 124 then | ||
| vim.schedule(function() | ||
| vim.notify('⚠ LDAP timeout for: ' .. name, vim.log.levels.WARN) | ||
| end) | ||
| cb(nil) | ||
| return | ||
| end | ||
| local line = vim.split(result.stdout or '', '\n')[1] or '' | ||
| local email2 = vim.trim(vim.split(line, '\t')[1] or '') | ||
| if email2:find('@') then | ||
| local p2 = vim.split(r.normalize_name(name), ' ', { trimempty = true }) | ||
| save_to_khard(p2[1] or name, p2[#p2] or '', email2) | ||
| end | ||
| cb(email2:find('@') and email2 or nil) | ||
| end) |
There was a problem hiding this comment.
Since ldap_owa_query is an optional/last-resort external command, it may not be installed on all systems. If it is missing, vim.system will throw an unhandled error and crash the resolver. Wrapping vim.system in pcall and invoking cb(nil) on failure ensures the resolver can gracefully fall back and continue.
local ok = pcall(vim.system,
{ 'ldap_owa_query', p[1] or norm2, p[2] or '', 'work' },
{ text = true, timeout = 10000 },
function(result)
if result.code == 124 then
vim.schedule(function()
vim.notify('⚠ LDAP timeout for: ' .. name, vim.log.levels.WARN)
end)
cb(nil)
return
end
local line = vim.split(result.stdout or '', '\n')[1] or ''
local email2 = vim.trim(vim.split(line, '\t')[1] or '')
if email2:find('@') then
local p2 = vim.split(r.normalize_name(name), ' ', { trimempty = true })
save_to_khard(p2[1] or name, p2[#p2] or '', email2)
end
cb(email2:find('@') and email2 or nil)
end)
if not ok then
cb(nil)
end
| vim.system( | ||
| { 'khard', 'email', '--parsable', '--remove-first-line', khard_queries[i] }, | ||
| { text = true }, | ||
| function(result) | ||
| local email = r.parse_khard(result.stdout) | ||
| if email then cb(email) else try_khard(i + 1) end | ||
| end) |
There was a problem hiding this comment.
If khard is not installed or fails to execute, vim.system will throw an unhandled error, halting the resolution process. Wrapping this call in pcall and falling back to try_khard(i + 1) on failure ensures the resolver remains robust.
local ok = pcall(vim.system,
{ 'khard', 'email', '--parsable', '--remove-first-line', khard_queries[i] },
{ text = true },
function(result)
local email = r.parse_khard(result.stdout)
if email then cb(email) else try_khard(i + 1) end
end)
if not ok then
try_khard(i + 1)
end
| for i, l in ipairs(lines) do | ||
| if l:match('^> ?%-%-') then | ||
| start = i | ||
| if l:match('^>[ >]*%-%-') then |
There was a problem hiding this comment.
The pattern ^>[ >]*%-%- matches any line starting with quotes and at least two dashes. This can lead to false positives, such as matching nested markdown horizontal rules (e.g., > > ---) or text lines starting with dashes, resulting in unintended deletion of quoted content. Enforce that the signature separator consists of exactly two dashes followed only by optional whitespace until the end of the line.
if l:match('^>[ >]*%-%-%s*$') then
| local ok, data = pcall(vim.json.decode, output) | ||
| if not ok or not data then return {} end |
There was a problem hiding this comment.
If vim.json.decode succeeds but returns a non-table value (such as a number, string, or boolean), calling ipairs(data) on line 87 will throw a runtime error. Checking that data is a table before iterating ensures robustness.
local ok, data = pcall(vim.json.decode, output)
if not ok or type(data) ~= 'table' then return {} end
| 'EMAIL;TYPE=WORK:' .. email, | ||
| 'END:VCARD', | ||
| }, '\n') | ||
| vim.system({ 'khard', 'add', '--input-format=vcard' }, { text = true, stdin = vcard }) |
There was a problem hiding this comment.
| e = vim.trim(e) | ||
| if not e:find('@') then names[#names + 1] = e end |
There was a problem hiding this comment.
If an entry consists only of whitespace, vim.trim(e) will return an empty string "". This empty string will be added to names and trigger unnecessary external process queries (like khard or notmuch) with an empty query. Check that e is not empty before adding it to names.
e = vim.trim(e)
if e ~= '' and not e:find('@') then names[#names + 1] = e end
3a518b1 to
781a9b1
Compare
|
Bot's findings on this PR reference This PR's diff is limited to the Telescope reply-all wiring. No changes needed here. |
- Extract pure address helpers into lua/nvim-mail/reply.lua (extract_email, split_addresses, build_reply_all_cc) - build_reply_all_cc merges original To+Cc, strips self (my_from) with case-insensitive email matching, and deduplicates - Ctrl+r handler reads hdrs.To and hdrs.Cc from notmuch JSON, emits Cc: header only when non-empty - 8 new unit tests covering all edge cases (no Cc, self-strip, Name <email> form, dedup, case-insensitive, only-self)
781a9b1 to
9a0dded
Compare
Summary
Closes #7
Ctrl+rin the Telescope mail picker now builds a reply-all draft instead of a reply-only draft.Changes
lua/nvim-mail/reply.lua(new): pure module withbuild_reply_all_cc(orig_to, orig_cc, my_from)— merges original To+Cc, strips self with case-insensitiveName <email>matching, deduplicates.lua/telescope/_extensions/nvim_mail.lua: readshdrs.Toandhdrs.Ccfrom notmuch JSON; callsreply.build_reply_all_cc; emitsCc:only when non-empty; updates prompt title to sayC-r:reply-all.tests/mail/telescope_spec.lua: 8 new unit tests covering no-Cc, self-strip,Name <email>form, dedup, case-insensitive match, only-self.UX choice
Direct reply-all (no prompt). Smaller diff, no extra keypress for the common case. A separate
Ctrl+Rfor reply-only can be added later if needed.Tests