From 6d34267f1cd0eadc78e0782a9f62546ff0a1e960 Mon Sep 17 00:00:00 2001
From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com>
Date: Sat, 20 Dec 2025 14:52:05 +0000
Subject: [PATCH] Address code review feedback: improve comments and test
 cleanup

Co-authored-by: Kinneyzhang <38454496+Kinneyzhang@users.noreply.github.com>
---
 tp-tests.el |  7 +++++--
 tp.el       | 12 ++++++------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/tp-tests.el b/tp-tests.el
index e468a22..4ec586d 100644
--- a/tp-tests.el
+++ b/tp-tests.el
@@ -2845,8 +2845,11 @@ incorrectly generate an anonymous tp-name instead of using the layer name."
 (ert-deftest tp-test-redefine-layer-updates-watchers ()
   "Test that re-defining a layer updates :watch correctly."
   (tp-test-with-temp-buffer
+    ;; Use defvar to create dynamically-bound variables that watcher callbacks can access
     (defvar tp-test-watch-log-old nil "Log for old watcher.")
     (defvar tp-test-watch-log-new nil "Log for new watcher.")
+    (setq tp-test-watch-log-old nil)
+    (setq tp-test-watch-log-new nil)
     (unwind-protect
         (progn
           ;; First definition with old watcher
@@ -2968,8 +2971,8 @@ incorrectly generate an anonymous tp-name instead of using the layer name."
             :props (face (:background $tp-test-applied-color))
             :data ((tp-test-applied-color . "blue")))
           ;; The text should now have the new color
-          ;; Note: This happens because re-definition updates the variable,
-          ;; which triggers the reactive update mechanism
+          ;; This happens because tp-define-layer calls tp--update-layer-regions
+          ;; at the end to update all text regions with the new properties
           (should (equal (plist-get (get-text-property 1 'face) :background) "blue")))
       ;; Cleanup
       (ignore-errors (makunbound 'tp-test-applied-color)))))
diff --git a/tp.el b/tp.el
index 89cc0c5..b59d792 100644
--- a/tp.el
+++ b/tp.el
@@ -2072,7 +2072,7 @@ The layer is stored in `tp-layer-alist'."
     (if (or all-reactive-syms data compute)
         ;; Has reactive features - register dependencies and resolve at runtime
         `(progn
-           ;; Clean up old dependencies first (for re-definition)
+           ;; Clean up old reactive dependencies, watchers, computed properties, and data (for re-definition)
            (tp--unregister-reactive-deps ',name)
            ;; Ensure all reactive variables are defined
            (tp--ensure-reactive-variables ',all-vars-to-define)
@@ -2098,7 +2098,7 @@ The layer is stored in `tp-layer-alist'."
            (assoc ',name tp-layer-alist))
       ;; No reactive symbols - use static properties
       `(progn
-         ;; Clean up old dependencies first (for re-definition from reactive to non-reactive)
+         ;; Clean up old reactive dependencies, watchers, computed properties, and data (for re-definition)
          (tp--unregister-reactive-deps ',name)
          (tp--set-layer-props ',name ',properties)
          ;; Update any text regions that already have this layer applied
@@ -2265,7 +2265,7 @@ and the group itself is stored in `tp-layer-groups'."
             (if (or all-reactive-syms data compute)
                 ;; Has reactive features - register dependencies and resolve at runtime
                 (push `(progn
-                         ;; Clean up old dependencies first (for re-definition)
+                         ;; Clean up old reactive dependencies, watchers, computed properties, and data (for re-definition)
                          (tp--unregister-reactive-deps ',layer-name)
                          ;; Ensure all reactive variables are defined
                          (tp--ensure-reactive-variables ',all-vars-to-define)
@@ -2290,7 +2290,7 @@ and the group itself is stored in `tp-layer-groups'."
                       layer-defs)
               ;; No reactive symbols - use static properties
               (push `(progn
-                       ;; Clean up old dependencies first (for re-definition)
+                       ;; Clean up old reactive dependencies, watchers, computed properties, and data (for re-definition)
                        (tp--unregister-reactive-deps ',layer-name)
                        (tp--set-layer-props ',layer-name ',props)
                        ;; Update any text regions that already have this layer applied
@@ -2305,7 +2305,7 @@ and the group itself is stored in `tp-layer-groups'."
             (if reactive-syms
                 ;; Has reactive symbols - register dependencies and resolve at runtime
                 (push `(progn
-                         ;; Clean up old dependencies first (for re-definition)
+                         ;; Clean up old reactive dependencies, watchers, computed properties, and data (for re-definition)
                          (tp--unregister-reactive-deps ',layer-name)
                          (tp--ensure-reactive-variables
                           ',(mapcar #'tp--reactive-var-symbol reactive-syms))
@@ -2318,7 +2318,7 @@ and the group itself is stored in `tp-layer-groups'."
                       layer-defs)
               ;; No reactive symbols - use static properties
               (push `(progn
-                       ;; Clean up old dependencies first (for re-definition)
+                       ;; Clean up old reactive dependencies, watchers, computed properties, and data (for re-definition)
                        (tp--unregister-reactive-deps ',layer-name)
                        (tp--set-layer-props ',layer-name ',props)
                        ;; Update any text regions that already have this layer applied