refactor(video-capture): scope capture URL to a dynamic binding

quick-video-capture passed the org-protocol URL through a global cj/video-download-current-url: the protocol handler setq the global, the capture handler read and cleared it. If a capture was aborted or errored between those steps, the stale URL survived into the next manual capture.

Renamed it to cj/--video-download-url and let-bind it around the org-capture call in the protocol handler instead of setq-ing a global. The binding lives only for the dynamic extent of the capture, so the handler still sees the URL while the capture runs, and an abort or error unwinds the binding automatically — no stale state, no manual clear. The handler still prompts when invoked manually with no URL bound. Tests cover the bound-URL download, the manual prompt, the empty-URL error, that the URL is visible during the capture, and that an aborted capture leaves nothing behind.

2 files changed, 90 insertions, 13 deletions

diff --git a/modules/quick-video-capture.el b/modules/quick-video-capture.el
index 0e9567dd..d74082e3 100644
--- a/modules/quick-video-capture.el
+++ b/modules/quick-video-capture.el
@@ -35,8 +35,12 @@
   "JavaScript bookmarklet for triggering video downloads from the browser.
 Add this as a bookmark in your browser to enable one-click video downloads.")
 
-(defvar cj/video-download-current-url nil
-  "Temporary storage for URL passed via org-protocol.")
+(defvar cj/--video-download-url nil
+  "URL for the active video capture, dynamically bound around `org-capture'.
+The org-protocol entry point `let'-binds this for the dynamic extent of
+its capture call, so the capture handler sees the URL while the capture
+runs, and an aborted or erroring capture unwinds the binding instead of
+leaving a stale URL for the next manual capture.")
 
 (defvar cj/video-download-initialized nil
   "Flag to track if video download has been initialized.")
@@ -46,23 +50,21 @@ Add this as a bookmark in your browser to enable one-click video downloads.")
 INFO is a plist containing :url from the org-protocol call."
   ;; Ensure we're initialized when called via protocol
   (cj/ensure-video-download-initialized)
-  (let ((url (plist-get info :url)))
-    (when url
-      ;; Store the URL for the capture template to use
-      (setq cj/video-download-current-url url))
-    ;; Trigger the capture
-    (org-capture nil "v")
-    nil))  ; Return nil to indicate we handled it
+  ;; Bind the URL for the dynamic extent of the capture call only.  The
+  ;; capture handler reads it while the capture runs; an aborted or
+  ;; erroring capture unwinds this binding, so nothing stale survives.
+  (let ((cj/--video-download-url (plist-get info :url)))
+    (org-capture nil "v"))
+  nil)  ; Return nil to indicate we handled it
 
 (defun cj/video-download-capture-handler ()
   "Handle video download during org-capture.
-This function is called from the capture template."
+This function is called from the capture template.  It uses the URL
+bound by the org-protocol entry point, or prompts when invoked manually."
   ;; Ensure media-utils is loaded when we actually need to download
   (require 'media-utils)
-  (let ((url (or cj/video-download-current-url
+  (let ((url (or cj/--video-download-url
                  (read-string "Video URL: "))))
-    ;; Clear the stored URL after using it
-    (setq cj/video-download-current-url nil)
     (if (string-empty-p url)
         (error "No URL provided for download")
       (cj/yt-dl-it url)
diff --git a/tests/test-quick-video-capture--url-state.el b/tests/test-quick-video-capture--url-state.el
new file mode 100644
index 00000000..6a0cd583
--- /dev/null
+++ b/tests/test-quick-video-capture--url-state.el
@@ -0,0 +1,75 @@
+;;; test-quick-video-capture--url-state.el --- Tests for video-capture URL state -*- lexical-binding: t; -*-
+
+;;; Commentary:
+;; Unit tests for the URL handoff between the org-protocol entry point and the
+;; capture handler in quick-video-capture.el.  The URL is passed via a
+;; dynamically-bound variable scoped to the org-capture call, so an aborted or
+;; erroring capture cannot leave a stale URL behind for the next manual capture.
+
+;;; Code:
+
+(require 'ert)
+(require 'cl-lib)
+
+(add-to-list 'load-path (expand-file-name "modules" user-emacs-directory))
+(require 'quick-video-capture)
+
+;;; cj/video-download-capture-handler
+
+(ert-deftest test-qvc-handler-downloads-bound-url ()
+  "Normal: the handler downloads the dynamically-bound URL and saves nothing."
+  (let (downloaded)
+    (cl-letf (((symbol-function 'require) (lambda (&rest _) nil))
+              ((symbol-function 'cj/yt-dl-it) (lambda (u) (setq downloaded u))))
+      (let ((cj/--video-download-url "https://example.com/v"))
+        (should (equal "" (cj/video-download-capture-handler))))
+      (should (equal "https://example.com/v" downloaded)))))
+
+(ert-deftest test-qvc-handler-prompts-when-no-bound-url ()
+  "Boundary: with no bound URL the handler prompts the user."
+  (let (downloaded)
+    (cl-letf (((symbol-function 'require) (lambda (&rest _) nil))
+              ((symbol-function 'cj/yt-dl-it) (lambda (u) (setq downloaded u)))
+              ((symbol-function 'read-string) (lambda (&rest _) "https://typed/v")))
+      (let ((cj/--video-download-url nil))
+        (cj/video-download-capture-handler))
+      (should (equal "https://typed/v" downloaded)))))
+
+(ert-deftest test-qvc-handler-empty-url-errors ()
+  "Error: an empty URL signals rather than downloading nothing."
+  (cl-letf (((symbol-function 'require) (lambda (&rest _) nil))
+            ((symbol-function 'read-string) (lambda (&rest _) "")))
+    (let ((cj/--video-download-url ""))
+      (should-error (cj/video-download-capture-handler)))))
+
+;;; cj/org-protocol-video-download — no global leak
+
+(ert-deftest test-qvc-protocol-binds-url-during-capture ()
+  "Normal: the protocol handler makes the URL visible to the capture call."
+  (let (seen-during)
+    (cl-letf (((symbol-function 'cj/ensure-video-download-initialized) #'ignore)
+              ((symbol-function 'org-capture)
+               (lambda (&rest _) (setq seen-during cj/--video-download-url))))
+      (cj/org-protocol-video-download '(:url "https://example.com/p")))
+    (should (equal "https://example.com/p" seen-during))))
+
+(ert-deftest test-qvc-protocol-leaves-no-stale-url ()
+  "Boundary: after the protocol capture returns, no URL remains bound."
+  (cl-letf (((symbol-function 'cj/ensure-video-download-initialized) #'ignore)
+            ((symbol-function 'org-capture) #'ignore))
+    (cj/org-protocol-video-download '(:url "https://example.com/p")))
+  (should (null cj/--video-download-url)))
+
+(ert-deftest test-qvc-protocol-aborted-capture-clears-url ()
+  "Error: a capture that errors/aborts mid-flow still leaves no stale URL.
+This is the regression the dynamic binding prevents: the old global
+setq survived an interrupted capture into the next manual one."
+  (cl-letf (((symbol-function 'cj/ensure-video-download-initialized) #'ignore)
+            ((symbol-function 'org-capture)
+             (lambda (&rest _) (error "simulated capture abort"))))
+    (ignore-errors
+      (cj/org-protocol-video-download '(:url "https://example.com/p"))))
+  (should (null cj/--video-download-url)))
+
+(provide 'test-quick-video-capture--url-state)
+;;; test-quick-video-capture--url-state.el ends here