client/ui/quickactions.go (1)

101-163: Fix the pause/resume loop to avoid pegging the CPU and freezing the UI

Once executeCommand sends to pauseChan, the poller flips getConnectionStatus to false and your default branch immediately hits continue. Because there is no matching resume send (the only call is commented out) the loop stays “paused” forever, yet it now busy-spins with no sleep, burning a core and never emitting another state. On top of that, if the connect/disconnect command returns an error the UI stays disabled and the user can’t retry.

Please rework this section to block on a ticker while paused and ensure you always resume the poller (both after success and on error). A concrete fix could look like:

-	go func() {
-		getConnectionStatus := true
-		for {
-			select {
-			case <-viewModel.pauseChan:
-				getConnectionStatus = false
-				log.Debug("uiChan paused.")
-			case <-viewModel.resumeChan:
-				getConnectionStatus = true
-				log.Debug("uiChan resumed.")
-			default:
-				if !getConnectionStatus {
-					continue
-				}
-				…
-				time.Sleep(500 * time.Millisecond)
-			}
-		}
-	}()
+	go func() {
+		ticker := time.NewTicker(500 * time.Millisecond)
+		defer ticker.Stop()
+		paused := false
+		for {
+			select {
+			case <-viewModel.pauseChan:
+				paused = true
+				log.Debug("uiChan paused.")
+			case <-viewModel.resumeChan:
+				paused = false
+				log.Debug("uiChan resumed.")
+			case <-ticker.C:
+				if paused {
+					continue
+				}
+				…
+			}
+		}
+	}()

and in executeCommand:

	q.pauseChan <- struct{}{}
	err := command.execute()
	if err != nil {
		log.Errorf("Status: Error - %v", err)
-	} else {
-		uiState = newQuickActionsUiState()
-		uiState.isConnectionChanged = true
-		q.uiChan <- uiState
-		//q.resumeChan <- struct{}{}
-	}
+		q.resumeChan <- struct{}{}
+		return
+	}
+	uiState = newQuickActionsUiState()
+	uiState.isConnectionChanged = true
+	q.uiChan <- uiState
+	q.resumeChan <- struct{}{}

Without these fixes the new window can hang the process and leaves users with a dead quick-actions button.

client/ui/signal_windows.go (2)

63-65: Align the log message with the quick actions handler.

The log still refers to a “Debug handler,” which is confusing when this code wires up the quick actions event listener. Please update the message to reflect the actual handler name.

-	log.Infof("Debug handler waiting for signal on event: %s", quickActionsTriggerEventName)
+	log.Infof("Quick actions handler waiting for signal on event: %s", quickActionsTriggerEventName)

---

159-170: Close the event handle and return contextual errors.

We still leak the event handle here—after SetEvent we never call CloseHandle, so every trigger leaves a live handle behind. While fixing that, please wrap the returned errors with context (this also addresses the prior feedback on this block). Remember to add the missing fmt import.

-	eventHandle, err := windows.OpenEvent(desiredAccesses, false, eventNamePtr)
-	if err != nil {
-		return err
-	}
-
-	err = windows.SetEvent(eventHandle)
-	if err != nil {
-		log.Printf("Error setting event: %v", err)
-		return err
-	}
-
-	return nil
+	eventHandle, err := windows.OpenEvent(desiredAccesses, false, eventNamePtr)
+	if err != nil {
+		return fmt.Errorf("open quick actions event '%s': %w", quickActionsTriggerEventName, err)
+	}
+	defer func() {
+		if cerr := windows.CloseHandle(eventHandle); cerr != nil {
+			log.Errorf("failed to close quick actions event handle '%s': %v", quickActionsTriggerEventName, cerr)
+		}
+	}()
+
+	if err := windows.SetEvent(eventHandle); err != nil {
+		return fmt.Errorf("set quick actions event '%s': %w", quickActionsTriggerEventName, err)
+	}
+
+	return nil

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

client/ui/quickactions.go (2)

209-221: Still unresolved: nil logo causes UI corruption for unhandled states.

The variables logo, buttonText, and buttonIcon remain uninitialized for connection states other than "Connected" and "Idle" (e.g., "Connecting", "Disconnected", "Error"). When an unhandled state arrives, logo stays nil, and line 248's assignment content.Objects[1] = logo corrupts the UI or risks a panic.

Initialize these variables to the disconnected assets before the conditional checks:

-	var logo *canvas.Image
-	var buttonText string
-	var buttonIcon fyne.Resource
+	logo := disconnectedImage
+	buttonText := disconnectedLabelText
+	buttonIcon := disconnectedCircleRes
 
 	if uiState.connectionStatus == string(internal.StatusConnected) {
 		buttonText = connectedLabelText
 		buttonIcon = connectedCircleRes
 		logo = connectedImage
 	} else if uiState.connectionStatus == string(internal.StatusIdle) {
-		buttonText = disconnectedLabelText
-		buttonIcon = disconnectedCircleRes
-		logo = disconnectedImage
+		// keep defaults
 	}

This ensures any unhandled state displays the greyed-out logo and safe button values.

---

281-282: Still unresolved: channel is never closed, preventing clean shutdown.

The uiChan is created but never closed. When vmCtx is cancelled (via window close at line 260), the watchConnectionStatus goroutine stops sending, but the channel remains open indefinitely. The consumer goroutine at lines 313-337 will block forever on the channel receive (line 319) after context cancellation since it won't receive the close signal.

Close the channel when the view model's work is done:

 	uiChan := make(chan quickActionsUiState, 1)
+	defer close(uiChan)
 	newQuickActionsViewModel(vmCtx, daemonClientConnectionStatusProvider{client: client}, connCmd, disConnCmd, uiChan)

Alternatively, close it inside watchConnectionStatus when the context is cancelled. Consider using sync.Once if multiple paths could close the channel.

client/ui/quickactions.go (1)

313-337: Consider extracting the UI update loop to a named method.

The anonymous goroutine spans 25 lines and could be extracted to a method like watchAndApplyUIStates(ctx, uiChan, ...) to improve readability and make the showQuickActionsUI method more scannable.

Based on learnings.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

client/ui/quickactions.go (2)

285-286: Close the UI channel to prevent goroutine leak.

The uiChan channel is never closed, which can prevent the UI update goroutine (lines 317-343) from terminating gracefully. While context cancellation stops the view model from sending, the receiver should be notified that no more values will arrive.

Close the channel when the view model context is done:

 	uiChan := make(chan quickActionsUiState, 1)
+	defer close(uiChan)
 	newQuickActionsViewModel(vmCtx, daemonClientConnectionStatusProvider{client: client}, connCmd, disConnCmd, uiChan)

Alternatively, if the view model should own the channel lifecycle, close it within the view model's cleanup logic and ensure proper synchronization with sync.Once to avoid double-close panics.

---

213-225: Default to disconnected assets for unhandled connection statuses.

The variables logo, buttonText, and buttonIcon are only initialized for "Connected" (line 217) and "Idle" (line 221) statuses. Other possible statuses like "Connecting", "Disconnected", or "Error" will leave these variables nil/empty. While the nil checks at lines 228, 232, and 251 prevent crashes, they result in no visual updates for unhandled statuses, leaving the window in a stale state.

A previous review comment suggested this was addressed, but the issue persists in the current code.

Initialize these variables with disconnected defaults at the top of the method:

 func (s *serviceClient) applyQuickActionsUiState(
 	uiState quickActionsUiState,
 	components quickActionsUiComponents,
 ) bool {
 	if uiState.isConnectionChanged {
 		fyne.DoAndWait(func() {
 			s.wQuickActions.Close()
 		})
 		return true
 	}
 
-	var logo *canvas.Image
-	var buttonText string
-	var buttonIcon fyne.Resource
+	// Default to disconnected state for unknown statuses
+	logo := components.disconnectedImage
+	buttonText := components.disconnectedLabelText
+	buttonIcon := components.disconnectedCircleRes
 
 	if uiState.connectionStatus == string(internal.StatusConnected) {
 		buttonText = components.connectedLabelText
 		buttonIcon = components.connectedCircleRes
 		logo = components.connectedImage
-	} else if uiState.connectionStatus == string(internal.StatusIdle) {
-		buttonText = components.disconnectedLabelText
-		buttonIcon = components.disconnectedCircleRes
-		logo = components.disconnectedImage
 	}

go.mod (1)

162-167: Verify transitive dependency scope for quick actions feature.

Multiple new indirect dependencies have been added, likely from Fyne and related UI libraries (lines 162–167 for Fyne ecosystem, 183–184 for IndexedDB/SafeJS, 191 for localization, 195 for bitmap, 214 for image resize, 215 for i18n, 231 for XDG portal, 241 for markdown, 250 for image processing). While many are expected for a new UI feature, confirm none are unnecessary bloat.

Consider documenting in the PR description or a comment which dependencies were added directly for the quick actions feature vs. which are transitive. This helps maintain awareness of supply chain scope. For example:

• Direct dependencies: fyne/v2 (UI), systray (system integration)
• Transitive/indirect: Fyne ecosystem libs, image processing, localization, XDG portal (Linux integration)

This isn't a blocker, but transparency aids future maintenance.

Also applies to: 175-175, 183-184, 191-191, 195-195, 214-215, 231-231, 241-241, 250-250

client/ui/quickactions.go (3)

147-167: Clarify the empty connection status during command execution.

At line 151, connectionStatus is set to an empty string, which doesn't match either "Connected" or "Idle". When this state reaches applyQuickActionsUiState (lines 217-225), the variables logo, buttonText, and buttonIcon will remain uninitialized (nil/empty), though the UI update logic checks for nil before applying. However, this makes the intent unclear.

Consider either:

1. Explicitly setting connectionStatus to the current known status, or
2. Adding a comment explaining that empty status intentionally leaves the logo unchanged during command execution

---

242-246: Consider caching the OnTapped handler to avoid repeated reassignment.

The OnTapped handler is reassigned on every UI state update (potentially every second during polling). While functionally correct, this is inefficient when toggleAction hasn't changed.

You could track whether toggleAction has changed and only reassign the handler when necessary, though the current approach is simple and works.

---

317-343: Consider extracting the UI update goroutine to a named function.

The inline goroutine spanning lines 317-343 handles UI state updates. Extracting it to a named method would improve readability and testability.

For example:

// In showQuickActionsUI, replace the inline goroutine:
go s.runQuickActionsUIUpdateLoop(vmCtx, uiChan, components)

// Add the method:
func (s *serviceClient) runQuickActionsUIUpdateLoop(
	ctx context.Context,
	uiChan <-chan quickActionsUiState,
	components quickActionsUiComponents,
) {
	for {
		select {
		case <-ctx.Done():
			return
		case uiState, ok := <-uiChan:
			if !ok {
				return
			}
			if s.applyQuickActionsUiState(uiState, components) {
				return
			}
		}
	}
}

This addresses a previous reviewer's suggestion for improved code organization.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro