diff options
| -rw-r--r-- | modules/mu4e-org-contacts-integration.el | 7 | ||||
| -rw-r--r-- | modules/org-roam-config.el | 22 | ||||
| -rw-r--r-- | modules/org-webclipper.el | 31 | ||||
| -rw-r--r-- | tests/test-org-roam-config-copy-todo-to-today.el | 15 | ||||
| -rw-r--r-- | tests/test-org-webclipper-commands.el | 16 | ||||
| -rw-r--r-- | todo.org | 8 |
6 files changed, 70 insertions, 29 deletions
diff --git a/modules/mu4e-org-contacts-integration.el b/modules/mu4e-org-contacts-integration.el index 8ffdccd2..68f83dd2 100644 --- a/modules/mu4e-org-contacts-integration.el +++ b/modules/mu4e-org-contacts-integration.el @@ -13,6 +13,13 @@ (require 'mu4e nil t) (require 'org-contacts nil t) +;; Cross-module symbols this file references. Declared so byte-compile in +;; isolation doesn't warn about free variables / undefined functions; the +;; actual definitions live where named. +(eval-when-compile (defvar contacts-file)) ; user-constants.el +(declare-function cj/get-all-contact-emails ; org-contacts-config.el + "org-contacts-config" ()) + ;; ---------------------- Completion at Point Function ------------------------- (defun cj/org-contacts-completion-at-point () diff --git a/modules/org-roam-config.el b/modules/org-roam-config.el index d0851ee4..306c7fe2 100644 --- a/modules/org-roam-config.el +++ b/modules/org-roam-config.el @@ -77,14 +77,26 @@ (global-set-key (kbd "C-c n d") org-roam-dailies-map) (org-roam-db-autosync-mode)) -;; Move closed tasks to today's journal when marked done +;; Move closed tasks to today's journal when marked done. + +(defun cj/--org-roam-should-copy-completed-task-p () + "Return non-nil when the current org-state change should copy to today's journal. +Skips: +- Fileless buffers (org-capture, indirect buffers, temp Org buffers) +- The gcal.org buffer (synced from Google Calendar) +- State changes that didn't actually enter a done state +- State changes where the previous state was already done (avoid re-copy)" + (let ((file (buffer-file-name))) + (and file + (bound-and-true-p org-state) + (member org-state org-done-keywords) + (not (member org-last-state org-done-keywords)) + (not (string= file (expand-file-name gcal-file)))))) + (with-eval-after-load 'org (add-to-list 'org-after-todo-state-change-hook (lambda () - (when (and (member org-state org-done-keywords) - (not (member org-last-state org-done-keywords)) - ;; Don't run for gcal.org - it's synced from Google Calendar - (not (string= (buffer-file-name) (expand-file-name gcal-file)))) + (when (cj/--org-roam-should-copy-completed-task-p) (cj/org-roam-copy-todo-to-today))))) ;; ------------------------- Org Roam Insert Immediate ------------------------- diff --git a/modules/org-webclipper.el b/modules/org-webclipper.el index be4c1cb6..cf4cdbf0 100644 --- a/modules/org-webclipper.el +++ b/modules/org-webclipper.el @@ -68,12 +68,12 @@ (require 'org-web-tools) (require 'user-constants) ;; for webclipped-file - ;; Register the org-protocol handler - (add-to-list 'org-protocol-protocol-alist - '("webclip" - :protocol "webclip" - :function cj/org-protocol-webclip - :kill-client t)) + ;; The org-protocol handler registration lives in the + ;; `with-eval-after-load 'org-protocol' block at the bottom of + ;; this module -- that's the more robust home (it survives + ;; org-protocol being loaded before or after this module). Two + ;; registration sites would silently drift if the alist entry + ;; shape ever changes. ;; Add capture templates if not already present (unless (assoc "W" org-capture-templates) @@ -119,14 +119,23 @@ Returns the processed content as a string with: (defun cj/org-protocol-webclip (info) "Process org-protocol webclip requests. -INFO is a plist containing :url and :title from the org-protocol call." +INFO is a plist containing :url and :title from the org-protocol call. +Signals `user-error' when :url is missing, nil, empty, or non-string -- an +unexpected plist shape used to silently set the globals to nil and fail +downstream inside the capture handler with confusing messages." (cj/webclipper-ensure-initialized) (let ((url (plist-get info :url)) (title (plist-get info :title))) - (when url - ;; Store the URL and title for the capture template to use - (setq cj/webclip-current-url url - cj/webclip-current-title (or title "Untitled"))) + (unless (and (stringp url) (not (string-empty-p url))) + (user-error + "org-protocol webclip: expected non-empty :url string, got %S" url)) + (when (and title (not (stringp title))) + (user-error + "org-protocol webclip: :title must be a string when provided, got %S" + title)) + ;; Store the URL and title for the capture template to use + (setq cj/webclip-current-url url + cj/webclip-current-title (or title "Untitled")) ;; Trigger the capture (org-capture nil "W") nil)) ; Return nil to indicate we handled it diff --git a/tests/test-org-roam-config-copy-todo-to-today.el b/tests/test-org-roam-config-copy-todo-to-today.el index bcac5a26..c642020d 100644 --- a/tests/test-org-roam-config-copy-todo-to-today.el +++ b/tests/test-org-roam-config-copy-todo-to-today.el @@ -51,7 +51,8 @@ (setq org-state "DONE") ; Dynamic variable used by org-mode hooks (cl-letf (((symbol-function 'cj/org-roam-copy-todo-to-today) (lambda () (setq copy-function-called t)))) - (run-hooks 'org-after-todo-state-change-hook) + (let ((buffer-file-name "/tmp/test-org-roam-fake.org")) + (run-hooks 'org-after-todo-state-change-hook)) (should copy-function-called))) (test-org-roam-todo-hook-teardown))) @@ -67,7 +68,8 @@ (setq org-state "CANCELLED") ; Dynamic variable used by org-mode hooks (cl-letf (((symbol-function 'cj/org-roam-copy-todo-to-today) (lambda () (setq copy-function-called t)))) - (run-hooks 'org-after-todo-state-change-hook) + (let ((buffer-file-name "/tmp/test-org-roam-fake.org")) + (run-hooks 'org-after-todo-state-change-hook)) (should copy-function-called))) (test-org-roam-todo-hook-teardown))) @@ -96,7 +98,8 @@ (setq org-state "DONE") ; New state is DONE (cl-letf (((symbol-function 'cj/org-roam-copy-todo-to-today) (lambda () (setq copy-function-called t)))) - (run-hooks 'org-after-todo-state-change-hook) + (let ((buffer-file-name "/tmp/test-org-roam-fake.org")) + (run-hooks 'org-after-todo-state-change-hook)) (should copy-function-called))) (test-org-roam-todo-hook-teardown))) @@ -112,7 +115,8 @@ (setq org-state "DONE") ; New state is DONE (cl-letf (((symbol-function 'cj/org-roam-copy-todo-to-today) (lambda () (setq copy-function-called t)))) - (run-hooks 'org-after-todo-state-change-hook) + (let ((buffer-file-name "/tmp/test-org-roam-fake.org")) + (run-hooks 'org-after-todo-state-change-hook)) (should copy-function-called))) (test-org-roam-todo-hook-teardown))) @@ -128,7 +132,8 @@ (setq org-state "CANCELLED") ; New state is CANCELLED (cl-letf (((symbol-function 'cj/org-roam-copy-todo-to-today) (lambda () (setq copy-function-called t)))) - (run-hooks 'org-after-todo-state-change-hook) + (let ((buffer-file-name "/tmp/test-org-roam-fake.org")) + (run-hooks 'org-after-todo-state-change-hook)) (should copy-function-called))) (test-org-roam-todo-hook-teardown))) diff --git a/tests/test-org-webclipper-commands.el b/tests/test-org-webclipper-commands.el index 3871774c..57c7b5fc 100644 --- a/tests/test-org-webclipper-commands.el +++ b/tests/test-org-webclipper-commands.el @@ -29,19 +29,27 @@ ;;; cj/webclipper-ensure-initialized -(ert-deftest test-webclipper-ensure-initialized-registers-protocol-and-templates () - "Normal: first call sets up the protocol entry + W and w capture templates, -and flips the initialized flag." +(ert-deftest test-webclipper-ensure-initialized-registers-templates () + "Normal: first call sets up the W and w capture templates and flips the +initialized flag. Protocol registration lives in the +`with-eval-after-load 'org-protocol' block at the bottom of the module -- +asserted separately below in `test-webclipper-protocol-registered-via-after-load'." (let ((cj/webclipper-initialized nil) (org-protocol-protocol-alist nil) (org-capture-templates nil)) (cl-letf (((symbol-function 'require) (lambda (&rest _) t))) (cj/webclipper-ensure-initialized)) (should cj/webclipper-initialized) - (should (assoc "webclip" org-protocol-protocol-alist)) (should (assoc "W" org-capture-templates)) (should (assoc "w" org-capture-templates)))) +(ert-deftest test-webclipper-protocol-registered-via-after-load () + "Loading org-webclipper installs a `with-eval-after-load 'org-protocol' block +that registers the webclip entry. Providing `'org-protocol' fires the block." + (let ((org-protocol-protocol-alist nil)) + (provide 'org-protocol) + (should (assoc "webclip" org-protocol-protocol-alist)))) + (ert-deftest test-webclipper-ensure-initialized-is-idempotent () "Boundary: second call doesn't re-register or duplicate templates." (let ((cj/webclipper-initialized nil) @@ -1594,7 +1594,7 @@ Expected outcome: - Keep startup quiet; only check expensive/external requirements when the relevant command runs. -**** TODO [#B] Guard the org-roam completed-task hook around non-file buffers :bug:refactor: +**** 2026-05-16 Sat @ 03:44:45 -0500 Guarded org-roam completed-task hook with cj/--org-roam-should-copy-completed-task-p =org-roam-config.el= adds a global =org-after-todo-state-change-hook= that copies newly completed tasks to today's org-roam journal. The hook assumes the @@ -1721,7 +1721,7 @@ Recommended improvement: - Link this note from the Org workflow review task and the broader load-graph refactor. -**** TODO [#C] De-duplicate =org-protocol-protocol-alist= registration in =org-webclipper= :cleanup: +**** 2026-05-16 Sat @ 03:44:45 -0500 Removed duplicate org-protocol-protocol-alist registration in cj/webclipper-ensure-initialized The =webclip= protocol handler is registered twice: =modules/org-webclipper.el:72-76= inside =cj/webclipper-ensure-initialized= @@ -1733,7 +1733,7 @@ the alist entry shape ever changes. Pick one site -- the =with-eval-after-load= block is the more robust location -- and remove the other. -**** TODO [#C] Validate =:url= and =:title= in =cj/org-protocol-webclip= :safety: +**** 2026-05-16 Sat @ 03:44:45 -0500 Validated :url and :title in cj/org-protocol-webclip before stashing =modules/org-webclipper.el:124-125= extracts =:url= and =:title= from the incoming protocol plist with no type or nil check. An unexpected @@ -1752,7 +1752,7 @@ corrupt each other's state. Pass the data through a plist on =org-capture-plist=, a per-invocation closure, or a queue, instead of global mutation. -**** TODO [#C] Declare cross-module free variables in =mu4e-org-contacts-integration.el= :cleanup:quick: +**** 2026-05-16 Sat @ 03:44:45 -0500 Declared cross-module free vars in mu4e-org-contacts-integration.el The module reads =contacts-file= (defined in =user-constants.el=) and calls =cj/get-all-contact-emails= (defined in =org-contacts-config.el=) |