fix: skip ElementEffect callback on reattach when no signals changed (#23771)

Add passivate/activate lifecycle to Effect so that ElementEffect can
preserve tracked Usage instances across detach/attach cycles. On
reattach, activate checks whether any dependency has changed since
passivation: if so, the callback re-runs with isInitialRun=true; if
not, only the dependency listeners are re-registered without invoking
the callback, avoiding redundant work and confusing double invocations.

Fixes #23730

Author: heruan

flow-server/src/main/java/com/vaadin/flow/dom/ElementEffect.java

@@ -56,8 +56,8 @@
  */
 public final class ElementEffect implements Serializable {
     private final ContextualEffectAction effectFunction;
-    private boolean closed = false;
     private Effect effect = null;
+    private Registration attachRegistration;
     private Registration detachRegistration;
 
     public ElementEffect(Element owner, EffectAction effectFunction) {
@@ -69,7 +69,7 @@ public ElementEffect(Element owner, ContextualEffectAction effectFunction) {
         Objects.requireNonNull(effectFunction,
                 "Effect function cannot be null");
         this.effectFunction = effectFunction;
-        owner.addAttachListener(attach -> {
+        attachRegistration = owner.addAttachListener(attach -> {
             enableEffect(attach.getSource());
 
             detachRegistration = owner.addDetachListener(detach -> {
@@ -216,10 +216,14 @@ public static Registration effect(Element owner,
     }
 
     private void enableEffect(Element owner) {
-        if (closed) {
-            return;
+        if (effect != null) {
+            effect.activate();
+        } else {
+            createEffect(owner);
         }
+    }
 
+    private void createEffect(Element owner) {
         Component parentComponent = ComponentUtil.findParentComponent(owner)
                 .get();
         UI ui = parentComponent.getUI().get();
@@ -233,7 +237,6 @@ private void enableEffect(Element owner) {
             }
         };
 
-        assert effect == null;
         effect = new Effect(errorHandlingEffectFunction, command -> {
             if (UI.getCurrent() == ui) {
                 // Run immediately if on the same UI
@@ -255,14 +258,23 @@ private void enableEffect(Element owner) {
 
     private void disableEffect() {
         if (effect != null) {
-            effect.dispose();
-            effect = null;
+            effect.passivate();
         }
     }
 
     public void close() {
-        disableEffect();
-        closed = true;
+        if (effect != null) {
+            effect.dispose();
+            effect = null;
+        }
+        if (attachRegistration != null) {
+            attachRegistration.remove();
+            attachRegistration = null;
+        }
+        if (detachRegistration != null) {
+            detachRegistration.remove();
+            detachRegistration = null;
+        }
     }
 
     /**

flow-server/src/main/java/com/vaadin/flow/signals/impl/Effect.java

@@ -52,6 +52,7 @@ public class Effect implements Serializable {
 
     private SerializableExecutor dispatcher;
     private final List<Registration> registrations = new ArrayList<>();
+    private final List<UsageTracker.Usage> usages = new ArrayList<>();
 
     // Non-final to allow clearing when the effect is closed
     private @Nullable SerializableRunnable action;
@@ -60,6 +61,7 @@ public class Effect implements Serializable {
 
     private boolean firstRun = true;
     private volatile boolean invalidatedFromBackground = false;
+    private boolean passivated = false;
 
     /**
      * Creates a signal effect with the given action and the default dispatcher.
@@ -160,16 +162,19 @@ public Effect(ContextualEffectAction action,
     private void revalidate() {
         assert registrations.isEmpty();
 
-        if (action == null) {
-            // closed
+        if (action == null || passivated) {
+            // closed or passivated
             return;
         }
 
+        usages.clear();
+
         activeEffects.get().add(this);
         try {
             boolean[] hasSignalUsage = { false };
             UsageTracker.track(action, usage -> {
                 hasSignalUsage[0] = true;
+                usages.add(usage);
                 // avoid lambda to allow proper deserialization
                 TransientListener usageListener = new TransientListener() {
                     @Override
@@ -236,12 +241,62 @@ private void clearRegistrations() {
         registrations.clear();
     }
 
+    /**
+     * Passivates this effect by removing all dependency listeners while
+     * preserving the tracked usages. The effect can later be re-activated with
+     * {@link #activate()}, which will check if any tracked values have changed
+     * and only re-run the callback if needed.
+     */
+    public synchronized void passivate() {
+        clearRegistrations();
+        passivated = true;
+    }
+
+    /**
+     * Re-activates a previously passivated effect. If any tracked signal has
+     * changed since passivation, the effect callback is re-run with
+     * {@link EffectContext#isInitialRun()} returning {@code true}. If nothing
+     * has changed, the effect simply re-registers its dependency listeners
+     * without running the callback.
+     */
+    public synchronized void activate() {
+        if (action == null || !passivated) {
+            return;
+        }
+        passivated = false;
+        firstRun = true;
+
+        if (usages.isEmpty()
+                || usages.stream().anyMatch(UsageTracker.Usage::hasChanges)) {
+            // Something changed while passivated, do a full revalidation
+            usages.clear();
+            revalidate();
+        } else {
+            // Nothing changed, just re-register listeners. A change
+            // listener may still fire immediately if a change sneaks in
+            // between the hasChanges check and the onNextChange call,
+            // in which case firstRun is already set to true above.
+            for (UsageTracker.Usage usage : usages) {
+                registrations.add(usage.onNextChange(this::onDependencyChange));
+            }
+            if (!invalidateScheduled.get()) {
+                // No invalidation was scheduled, so no change was
+                // detected during re-registration. Reset firstRun for
+                // normal tracking. If an invalidation was scheduled,
+                // firstRun stays true and will be reset by the
+                // eventual revalidation.
+                firstRun = false;
+            }
+        }
+    }
+
     /**
      * Disposes this effect by unregistering all current dependencies and
      * preventing the action from running again.
      */
     public synchronized void dispose() {
         clearRegistrations();
+        usages.clear();
         action = null;
     }
 

flow-server/src/test/java/com/vaadin/flow/dom/ElementEffectTest.java

@@ -470,6 +470,81 @@ public void effect_componentAttachedAndDetached_effectEnabledAndDisabled() {
         assertEquals(3, count.get(), "Effect should not be run after remove");
     }
 
+    @Test
+    public void effect_reattachWithoutChanges_effectNotReRun() {
+        CurrentInstance.clearAll();
+        TestComponent component = new TestComponent();
+        ValueSignal<String> signal = new ValueSignal<>("initial");
+        AtomicInteger count = new AtomicInteger();
+        Signal.effect(component, () -> {
+            signal.get();
+            count.incrementAndGet();
+        });
+
+        MockUI ui = new MockUI();
+        ui.add(component);
+        assertEquals(1, count.get(), "Effect should run on attach");
+
+        ui.remove(component);
+        ui.add(component);
+        assertEquals(1, count.get(),
+                "Effect should not re-run on reattach when nothing changed");
+
+        signal.set("changed");
+        assertEquals(2, count.get(),
+                "Effect should still respond to changes after reattach");
+    }
+
+    @Test
+    public void effect_reattachWithChanges_effectReRunWithInitialRun() {
+        CurrentInstance.clearAll();
+        TestComponent component = new TestComponent();
+        ValueSignal<String> signal = new ValueSignal<>("initial");
+        List<Boolean> initialRuns = new ArrayList<>();
+        Signal.effect(component, ctx -> {
+            signal.get();
+            initialRuns.add(ctx.isInitialRun());
+        });
+
+        MockUI ui = new MockUI();
+        ui.add(component);
+        assertEquals(List.of(true), initialRuns);
+
+        signal.set("update");
+        assertEquals(List.of(true, false), initialRuns);
+
+        ui.remove(component);
+        signal.set("changed while detached");
+        ui.add(component);
+        assertEquals(List.of(true, false, true), initialRuns,
+                "Reattach with changes should run with isInitialRun=true");
+    }
+
+    @Test
+    public void effect_reattachWithoutChanges_nextChangeNotInitialRun() {
+        CurrentInstance.clearAll();
+        TestComponent component = new TestComponent();
+        ValueSignal<String> signal = new ValueSignal<>("initial");
+        List<Boolean> initialRuns = new ArrayList<>();
+        Signal.effect(component, ctx -> {
+            signal.get();
+            initialRuns.add(ctx.isInitialRun());
+        });
+
+        MockUI ui = new MockUI();
+        ui.add(component);
+        assertEquals(List.of(true), initialRuns);
+
+        ui.remove(component);
+        ui.add(component);
+        assertEquals(List.of(true), initialRuns,
+                "No re-run on reattach without changes");
+
+        signal.set("changed after reattach");
+        assertEquals(List.of(true, false), initialRuns,
+                "Normal change after reattach should not be initial run");
+    }
+
     @Test
     public void elementEffect_signalValueChanges_componentUpdated() {
         CurrentInstance.clearAll();