[client] Add quick actions window #4717
Conversation
It now displays the NetBird logo and a single button with a round icon
This also updates fyne to v2.7.0
…ngs-show-popup-macos
To use its clipboard rather than the window's when showing the upload success dialog
Settings now accept a function callback
…doromaraujo/netbird into feature/quick-settings-show-popup-macos
client/ui/quickactions.go
Outdated
|
|
||
| toggleConnectionButton.OnTapped = func() { | ||
| go func() { | ||
| uiState.toggleAction() |
There was a problem hiding this comment.
Why do we set this in a goroutine? That looks like it can race with something else.
There was a problem hiding this comment.
It's the command to be used (connect, disconnect) when user taps the button. It's updated via the UI channel; it's also the only place where it is set.
| return | ||
| } | ||
|
|
||
| uiChan := make(chan quickActionsUiState, 1) |
There was a problem hiding this comment.
This channel should be closed, e.g. in a defer. Consider using sync.Once to avoid panics if it can be closed in more than one code path
There was a problem hiding this comment.
The goroutine that uses it is now making use of ctx.Done to return from it. More than one place in the view model send UI states to it.
client/ui/quickactions.go
Outdated
| provider clientConnectionStatusProvider | ||
| connect clientCommand | ||
| disconnect clientCommand | ||
| uiChan chan quickActionsUiState |
There was a problem hiding this comment.
This field is accessed from multiple goroutines but it's not protected
There was a problem hiding this comment.
Which field? What protection is needed?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a Quick Actions Fyne UI with platform-specific signal handlers to spawn/show it, a --quick-actions CLI flag, embedded quick-action and disconnected icons, periodic daemon status polling and connect/disconnect commands, and signal-sending logic to request existing daemons to display the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExistingDaemon as Existing Daemon
participant QuickProc as QuickActions Proc
participant gRPC as gRPC Daemon
User->>ExistingDaemon: sendShowWindowSignal / trigger event
ExistingDaemon->>ExistingDaemon: platform signal handler receives (SIGUSR1 / named event)
ExistingDaemon->>QuickProc: openQuickActions() → exec binary --quick-actions --daemon-addr
activate QuickProc
QuickProc->>QuickProc: showQuickActionsUI()
loop every 1s
QuickProc->>gRPC: Status()
gRPC-->>QuickProc: Status response
QuickProc->>QuickProc: update UI state (button, hint, icon)
end
User->>QuickProc: Click toggle
alt was connected
QuickProc->>gRPC: Disconnect command
else was disconnected
QuickProc->>gRPC: Connect command
end
gRPC-->>QuickProc: Ack/status update
QuickProc->>QuickProc: refresh UI
deactivate QuickProc
sequenceDiagram
participant Launcher as Initial Client Start
participant ServiceClient as serviceClient
participant SignalHandler as Platform Signal Handler
Launcher->>ServiceClient: start()
ServiceClient->>ServiceClient: parseFlags (includes --quick-actions)
ServiceClient->>SignalHandler: setupSignalHandler(ctx)
alt Another daemon running
ServiceClient->>SignalHandler: sendShowWindowSignal(pid)
SignalHandler->>ExistingDaemon: OS-level signal / named event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
Comment |
client/ui/quickactions.go
Outdated
| buttonText = disconnectedLabelText | ||
| buttonIcon = disconnectedCircle.Resource | ||
| logo = disconnectedImage | ||
| } |
There was a problem hiding this comment.
What about the other states? How do we handle those?
There was a problem hiding this comment.
Other states won't be displayed in this UI. It's either "Connect " when disconnected or "Disconnect" when connected, and when clicked the button will be disabled until it either switches connection status or an error occurs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
client/ui/quickactions.go (1)
101-163: Fix the pause/resume loop to avoid pegging the CPU and freezing the UIOnce
executeCommandsends topauseChan, the poller flipsgetConnectionStatusto false and yourdefaultbranch immediately hitscontinue. 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
SetEventwe never callCloseHandle, 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 missingfmtimport.- 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
client/ui/assets/netbird-disconnected.icois excluded by!**/*.icoclient/ui/assets/netbird-disconnected.pngis excluded by!**/*.pnggo.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
client/ui/client_ui.go(7 hunks)client/ui/debug.go(4 hunks)client/ui/icons.go(1 hunks)client/ui/icons_windows.go(1 hunks)client/ui/quickactions.go(1 hunks)client/ui/quickactions_assets.go(1 hunks)client/ui/signal_unix.go(1 hunks)client/ui/signal_windows.go(1 hunks)go.mod(9 hunks)
🧰 Additional context used
🪛 GitHub Check: JS / Lint
client/ui/client_ui.go
[failure] 124-124:
client.setDefaultFonts undefined (type *serviceClient has no field or method setDefaultFonts) (typecheck)
🔇 Additional comments (3)
go.mod (3)
31-32: Direct dependency updates align with quick actions feature addition.The Fyne UI framework (v2.7.0), systray, fsnotify, and testify updates are appropriate for the PR's quick actions window implementation. The versions are reasonable and stable.
Also applies to: 46-46, 85-85
32-32: I need to verify that this commit contains the required signal handling improvements for the quick actions feature. Let me check the actual commit details from the repository.Verify that the systray commit contains the signal handling improvements described.
The pseudo-version commit
ca66a66d8b58cfe0f18d2a1f999f2f9d47364606successfully resolves via Go's module system (confirmed: timestamp 2025-06-03T11:35:21Z), establishing that the commit exists and is accessible. However, I cannot verify through public sources that this commit contains the claimed "signal handling improvements for quick actions feature" as:
- The term "quick actions" does not appear in systray's public documentation or issue tracker
- Web searches for systray-related signal handling improvements return no relevant results
- The specific changes in this commit are not accessible through public channels
Since the commit's existence is confirmed but its contents cannot be independently verified, manually inspect the systray repository commit directly to confirm the signal handling improvements before merging.
160-165: No actionable changes required; Fyne transitive dependencies verified safe.Verification confirms:
- No known vulnerabilities detected in Fyne and related transitive dependencies (go-indexeddb, safejs, rymdport/portal, etc.)
- WASM dependencies properly constrained via
//go:build jstags and isolated toclient/wasm/package tree- Desktop code paths use
//go:build !jsconstraints, ensuring WASM-only dependencies are excluded from desktop binaries- Architecture correctly separates WebAssembly and desktop code, preventing bloat or unintended linking
The dependency additions from Fyne v2.7.0 upgrade are well-integrated and safe for production use.
| client.setupSignalHandler(client.ctx) | ||
|
|
||
| client.setDefaultFonts() | ||
| systray.Run(client.onTrayReady, client.onTrayExit) |
There was a problem hiding this comment.
Restore the missing setDefaultFonts implementation
client.setDefaultFonts() is invoked here, but no definition is provided in any of the included build targets, and the static analysis/typecheck run already fails with “client.setDefaultFonts undefined”. The PR will not compile until the function is (re)introduced or this call is removed/guarded. Please add the missing implementation (with the appropriate build tags) or drop the call so the build succeeds.
🧰 Tools
🪛 GitHub Check: JS / Lint
[failure] 124-124:
client.setDefaultFonts undefined (type *serviceClient has no field or method setDefaultFonts) (typecheck)
To address racing conditions, it also replaces usage of pause and resume channels with an atomic bool.
Also, format error when trying to set event on sendShowWindowSignal
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/ui/quickactions.go (2)
209-221: Still unresolved: nil logo causes UI corruption for unhandled states.The variables
logo,buttonText, andbuttonIconremain uninitialized for connection states other than"Connected"and"Idle"(e.g.,"Connecting","Disconnected","Error"). When an unhandled state arrives,logostaysnil, and line 248's assignmentcontent.Objects[1] = logocorrupts 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
uiChanis created but never closed. WhenvmCtxis cancelled (via window close at line 260), thewatchConnectionStatusgoroutine 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
watchConnectionStatuswhen the context is cancelled. Consider usingsync.Onceif multiple paths could close the channel.
🧹 Nitpick comments (1)
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 theshowQuickActionsUImethod more scannable.Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/ui/quickactions.go(1 hunks)client/ui/signal_windows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/ui/signal_windows.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/ui/quickactions.go (1)
client/proto/daemon_grpc.pb.go (1)
DaemonServiceClient(20-67)
🔇 Additional comments (7)
client/ui/quickactions.go (7)
1-5: LGTM!The build constraints and asset bundling directives are appropriate for this cross-platform UI feature.
26-86: LGTM!The type definitions follow clean architecture patterns (provider abstraction, command pattern) and the use of
atomic.Boolat line 85 properly handles concurrent access toisWatchingConnectionStatus.
88-104: LGTM!The view model initialization is well-structured: it sends an initial UI state for immediate feedback and properly passes context to the background polling goroutine.
106-129: LGTM!The status update logic is sound: it queries the daemon, handles errors gracefully by sending a disabled state, and appropriately assigns the toggle action based on connection status.
131-145: LGTM!The polling loop correctly implements context cancellation, properly cleans up the ticker, and respects the atomic flag to pause updates during command execution.
147-167: LGTM!The command execution properly disables polling during the operation and handles both success (closing window) and error (re-enabling polling) paths. The next ticker cycle will refresh the UI after an error, providing eventual consistency.
169-189: LGTM!The helper functions are well-implemented:
getSystemTrayNameprovides platform-specific terminology, andgetNetBirdImageproperly configures the image size and fill mode.
…ngs-show-popup-macos
to applyQuickActionsUiState function
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/ui/quickactions.go (2)
285-286: Close the UI channel to prevent goroutine leak.The
uiChanchannel 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.Onceto avoid double-close panics.
213-225: Default to disconnected assets for unhandled connection statuses.The variables
logo,buttonText, andbuttonIconare 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 }
🧹 Nitpick comments (4)
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,
connectionStatusis set to an empty string, which doesn't match either "Connected" or "Idle". When this state reachesapplyQuickActionsUiState(lines 217-225), the variableslogo,buttonText, andbuttonIconwill remain uninitialized (nil/empty), though the UI update logic checks for nil before applying. However, this makes the intent unclear.Consider either:
- Explicitly setting
connectionStatusto the current known status, or- 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
OnTappedhandler is reassigned on every UI state update (potentially every second during polling). While functionally correct, this is inefficient whentoggleActionhasn't changed.You could track whether
toggleActionhas 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.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
client/ui/quickactions.go(1 hunks)go.mod(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/ui/quickactions.go (1)
client/proto/daemon_grpc.pb.go (1)
DaemonServiceClient(20-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Build Cache
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
🔇 Additional comments (8)
go.mod (2)
31-32: Clarify why systray uses a commit hash instead of the stable release.The latest stable versions are fyne.io/fyne/v2 v2.7.0 and fyne.io/systray v1.11.0. Line 31 correctly uses fyne/v2 v2.7.0, but line 32 uses
v1.11.1-0.20250603113521-ca66a66d8b58—a pseudo-version pointing to a specific commit rather than the stable v1.11.0 release.Please confirm:
- Is this a fork or custom branch? If so, document the rationale and maintenance status.
- If it's a required fix from upstream that isn't in v1.11.0 yet, reference the issue/PR explaining why the commit is necessary.
31-31: I need to search for specific security advisories on each of the three updated dependencies. The previous query returned general Go ecosystem vulnerabilities but not the specific packages mentioned. Let me check each dependency:No known security vulnerabilities found in updated dependencies.
Verification via GitHub Advisory Database and targeted security searches confirms that fyne.io/fyne/v2 v2.7.0, fsnotify v1.9.0, and testify v1.11.1 have no known CVEs or published security advisories. These are all stable releases without reported security issues.
client/ui/quickactions.go (6)
1-39: LGTM: Well-structured state management.The
quickActionsUiStatestruct and constructor provide a clean foundation for managing UI state with appropriate default values.
49-58: Verify that 400ms timeout is sufficient for Status RPC.The 400ms timeout for the Status gRPC call is quite aggressive. On slower systems, under load, or with network latency, this could result in frequent timeout errors and a degraded user experience.
Consider whether a longer timeout (e.g., 1-2 seconds) would be more appropriate, or verify through testing that 400ms is sufficient across expected deployment scenarios.
60-78: LGTM: Clean command pattern.The command interface and implementations provide a nice abstraction for connect/disconnect actions.
80-104: LGTM: View model properly uses atomic operations.The view model correctly uses
atomic.Boolfor theisWatchingConnectionStatusflag, ensuring thread-safe access from multiple goroutines. The goroutine started at line 103 is properly managed via context cancellation.
106-145: LGTM: Proper ticker usage and context handling.The polling mechanism correctly uses a ticker with deferred cleanup and proper context cancellation, addressing previous reviewer concerns. The atomic flag prevents unnecessary status checks when commands are executing.
169-189: LGTM: Clean helper functions.Both utility functions are straightforward and correctly implemented.
|
3 participants
Describe your changes
Adds a window that will pop up when running NetBird on desktop after it is already running.
It sends a SIGUSR1 to a previously set up signal handler (platform-dependent) that will handle opening the window itself.
The window only contains the NetBird logo with a single button indicating the action to connect or disconnect.
When connected, the NetBird logo is colored, when disconnected, the logo is greyed out.
On connection change, the window closes on its own.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Improvements
Chores