fix(workspace): address copilot PR review feedback - add validation, constants, and documentation improvements

- Extract popup dimension constants and focus throttle interval to named constants
- Add direction validation in SplitPane and FocusNeighbor with error messages
- Improve WidgetUnparentChecked documentation with detailed return semantics
- Add child widget validation in WidgetSetFocusChild
- Clarify WidgetWaitForDraw comment as no-op stub (no synchronization guarantees)
- Add deprecation note to TreeRebalancer indicating temporary disablement

Note: Deferred complex refactoring suggestions (closePane, scheduleIdleGuarded TOCTOU, OAuth URL parsing) for separate focused review to avoid introducing regressions.

commit: adb5e50da6c6eabafbe2c029c165a0bb0d802518
author: Brice · 12mo ago · 2025-10-05 15:30 UTC
parent: 8ec5d1d
signature: unsigned

6 files · +43 -8

M internal/app/browser/workspace_navigation.go +6 -1


@@ -11,6 +11,11 @@ import (
11	11		"github.com/bnema/dumber/pkg/webkit"
12	12		)
13	13
	14	+	const (
	15	+	// focusThrottleInterval prevents rapid focus changes that could cause infinite loops
	16	+	focusThrottleInterval = 100 * time.Millisecond
	17	+	)
	18	+
14	19		// DispatchPaneFocusEvent sends a workspace focus event to a pane's webview
15	20		func (wm WorkspaceManager) DispatchPaneFocusEvent(node paneNode, active bool) {
16	21		if node == nil \|\| node.pane == nil \|\| node.pane.webView == nil {

@@ -69,7 +74,6 @@ func (wm WorkspaceManager) focusByView(view webkit.WebView) {
69	74
70	75		// Throttle focus changes to prevent infinite loops
71	76		wm.focusThrottleMutex.Lock()
72		-	const focusThrottleInterval = 100 * time.Millisecond
73	77		if time.Since(wm.lastFocusChange) < focusThrottleInterval {
74	78		wm.focusThrottleMutex.Unlock()
75	79		return

@@ -155,6 +159,7 @@ func (wm *WorkspaceManager) FocusNeighbor(direction string) bool {
155	159		case "left", "right":
156	160		return wm.focusAdjacent(strings.ToLower(direction))
157	161		default:
	162	+	log.Printf("[workspace] invalid focus direction: %s (expected: up, down, left, right)", direction)
158	163		return false
159	164		}
160	165		}

 	"github.com/bnema/dumber/pkg/webkit"
 )
+const (
+	// focusThrottleInterval prevents rapid focus changes that could cause infinite loops
+	focusThrottleInterval = 100 * time.Millisecond
+)
 // DispatchPaneFocusEvent sends a workspace focus event to a pane's webview
 func (wm *WorkspaceManager) DispatchPaneFocusEvent(node *paneNode, active bool) {
 	if node == nil || node.pane == nil || node.pane.webView == nil {
 	// Throttle focus changes to prevent infinite loops
 	wm.focusThrottleMutex.Lock()
-	const focusThrottleInterval = 100 * time.Millisecond
 	if time.Since(wm.lastFocusChange) < focusThrottleInterval {
 		wm.focusThrottleMutex.Unlock()
 		return
 	case "left", "right":
 		return wm.focusAdjacent(strings.ToLower(direction))
 	default:
+		log.Printf("[workspace] invalid focus direction: %s (expected: up, down, left, right)", direction)
 		return false
 	}
 }

M internal/app/browser/workspace_operations.go +11 -0


@@ -14,6 +14,17 @@ func (wm WorkspaceManager) SplitPane(target paneNode, direction string) (*pane
14	14		return nil, fmt.Errorf("workspace manager is nil")
15	15		}
16	16
	17	+	// Validate direction parameter
	18	+	validDirections := map[string]bool{
	19	+	"left": true,
	20	+	"right": true,
	21	+	"up": true,
	22	+	"down": true,
	23	+	}
	24	+	if !validDirections[direction] {
	25	+	return nil, fmt.Errorf("invalid split direction '%s', expected one of: left, right, up, down", direction)
	26	+	}
	27	+
17	28		log.Printf("[workspace] Starting split operation: target=%p direction=%s", target, direction)
18	29
19	30		// Step 1: Validate geometry constraints (only in DebugBasic+)


@@ -14,6 +14,17 @@ func (wm WorkspaceManager) SplitPane(target paneNode, direction string) (*pane
14	return nil, fmt.Errorf("workspace manager is nil")	14		return nil, fmt.Errorf("workspace manager is nil")
15	}	15		}
16		16
		17	+	// Validate direction parameter
		18	+	validDirections := map[string]bool{
		19	+	"left": true,
		20	+	"right": true,
		21	+	"up": true,
		22	+	"down": true,
		23	+	}
		24	+	if !validDirections[direction] {
		25	+	return nil, fmt.Errorf("invalid split direction '%s', expected one of: left, right, up, down", direction)
		26	+	}
		27	+
17	log.Printf("[workspace] Starting split operation: target=%p direction=%s", target, direction)	28		log.Printf("[workspace] Starting split operation: target=%p direction=%s", target, direction)
18		29
19	// Step 1: Validate geometry constraints (only in DebugBasic+)	30		// Step 1: Validate geometry constraints (only in DebugBasic+)

M internal/app/browser/workspace_popup.go +8 -2


@@ -11,6 +11,12 @@ import (
11	11		"github.com/bnema/dumber/pkg/webkit"
12	12		)
13	13
	14	+	const (
	15	+	// Default popup window dimensions - sized for OAuth flows
	16	+	defaultPopupMinWidth = 500 // Minimum width for OAuth popups
	17	+	defaultPopupMinHeight = 600 // Minimum height for OAuth popups
	18	+	)
	19	+
14	20		// HandlePopup handles popup window creation requests from WebViews
15	21		func (wm WorkspaceManager) HandlePopup(source webkit.WebView, url string) *webkit.WebView {
16	22		log.Printf("[workspace] HandlePopup called for URL: %s", url)

@@ -697,8 +703,8 @@ func (wm WorkspaceManager) configureRelatedPopup(sourceNode paneNode, webView
697	703
698	704		// applyPopupSizeConstraints applies minimum size constraints to prevent OAuth popup compression
699	705		func (wm WorkspaceManager) applyPopupSizeConstraints(view webkit.WebView, width, height int) {
700		-	minWidth := 500 // Default minimum width
701		-	minHeight := 600 // Default minimum height
	706	+	minWidth := defaultPopupMinWidth
	707	+	minHeight := defaultPopupMinHeight
702	708
703	709		// Use provided dimensions if valid
704	710		if width > 0 {

 	"github.com/bnema/dumber/pkg/webkit"
 )
+const (
+	// Default popup window dimensions - sized for OAuth flows
+	defaultPopupMinWidth  = 500 // Minimum width for OAuth popups
+	defaultPopupMinHeight = 600 // Minimum height for OAuth popups
+)
 // HandlePopup handles popup window creation requests from WebViews
 func (wm *WorkspaceManager) HandlePopup(source *webkit.WebView, url string) *webkit.WebView {
 	log.Printf("[workspace] HandlePopup called for URL: %s", url)
 // applyPopupSizeConstraints applies minimum size constraints to prevent OAuth popup compression
 func (wm *WorkspaceManager) applyPopupSizeConstraints(view *webkit.WebView, width, height int) {
-	minWidth := 500  // Default minimum width
+	minWidth := defaultPopupMinWidth
-	minHeight := 600 // Default minimum height
+	minHeight := defaultPopupMinHeight
 	// Use provided dimensions if valid
 	if width > 0 {

M internal/app/browser/workspace_tree_rebalancer.go +2 -0


@@ -32,6 +32,8 @@ const (
32	32		type TreeMetrics struct{}
33	33
34	34		// NewTreeRebalancer returns a disabled stub implementation.
	35	+	// NOTE: Tree rebalancing logic has been temporarily disabled in favor of manual geometry management.
	36	+	// Future work may restore automatic rebalancing or replace it with an alternative approach.
35	37		func NewTreeRebalancer(_ WorkspaceManager, _ TreeValidator) *TreeRebalancer {
36	38		return &TreeRebalancer{}
37	39		}


@@ -32,6 +32,8 @@ const (
32	type TreeMetrics struct{}	32		type TreeMetrics struct{}
33		33
34	// NewTreeRebalancer returns a disabled stub implementation.	34		// NewTreeRebalancer returns a disabled stub implementation.
		35	+	// NOTE: Tree rebalancing logic has been temporarily disabled in favor of manual geometry management.
		36	+	// Future work may restore automatic rebalancing or replace it with an alternative approach.
35	func NewTreeRebalancer(_ WorkspaceManager, _ TreeValidator) *TreeRebalancer {	37		func NewTreeRebalancer(_ WorkspaceManager, _ TreeValidator) *TreeRebalancer {
36	return &TreeRebalancer{}	38		return &TreeRebalancer{}
37	}	39		}

M pkg/webkit/workspace_widgets_cgo.go +12 -1


@@ -609,7 +609,14 @@ func WidgetUnparent(widget uintptr) {
609	609		}
610	610
611	611		// WidgetUnparentChecked safely detaches a widget from its parent if still valid.
612		-	// Returns true when the widget was unparented or already detached.
	612	+	//
	613	+	// Return semantics:
	614	+	// - Returns false if the widget pointer is zero or not a valid GTK widget (invalid input).
	615	+	// - Returns true if the widget has no parent (already detached).
	616	+	// - Returns true if the widget was successfully unparented (parent changed or is now nil).
	617	+	// - Returns false if the widget is still valid and the parent did not change (unparent failed).
	618	+	//
	619	+	// Callers can use the return value to distinguish between invalid input, already-detached widgets, and successful unparenting.
613	620		func WidgetUnparentChecked(widget uintptr) bool {
614	621		if widget == 0 {
615	622		log.Printf("[workspace] WidgetUnparentChecked: widget=0")

@@ -728,6 +735,10 @@ func WidgetSetFocusChild(widget uintptr, child uintptr) {
728	735		}
729	736		var childPtr *C.GtkWidget
730	737		if child != 0 {
	738	+	if !widgetIsValid(child) {
	739	+	log.Printf("[workspace] WidgetSetFocusChild skipped invalid child=%#x", child)
	740	+	return
	741	+	}
731	742		childPtr = (*C.GtkWidget)(unsafe.Pointer(child))
732	743		}
733	744		C.gtk_widget_set_focus_child((*C.GtkWidget)(unsafe.Pointer(widget)), childPtr)

 }
 // WidgetUnparentChecked safely detaches a widget from its parent if still valid.
-// Returns true when the widget was unparented or already detached.
+//
+// Return semantics:
+//   - Returns false if the widget pointer is zero or not a valid GTK widget (invalid input).
+//   - Returns true if the widget has no parent (already detached).
+//   - Returns true if the widget was successfully unparented (parent changed or is now nil).
+//   - Returns false if the widget is still valid and the parent did not change (unparent failed).
+//
+// Callers can use the return value to distinguish between invalid input, already-detached widgets, and successful unparenting.
 func WidgetUnparentChecked(widget uintptr) bool {
 	if widget == 0 {
 		log.Printf("[workspace] WidgetUnparentChecked: widget=0")
 	}
 	var childPtr *C.GtkWidget
 	if child != 0 {
+		if !widgetIsValid(child) {
+			log.Printf("[workspace] WidgetSetFocusChild skipped invalid child=%#x", child)
+			return
+		}
 		childPtr = (*C.GtkWidget)(unsafe.Pointer(child))
 	}
 	C.gtk_widget_set_focus_child((*C.GtkWidget)(unsafe.Pointer(widget)), childPtr)

M pkg/webkit/workspace_widgets_stub.go +4 -4


@@ -401,8 +401,10 @@ func WidgetGetBounds(widget uintptr) (WidgetBounds, bool) {
401	401		return stub.bounds, true
402	402		}
403	403
404		-	// WidgetWaitForDraw ensures widget operations are complete (GTK test pattern)
405		-	// This replaces the problematic IdleAdd pattern with proper widget synchronization
	404	+	// WidgetWaitForDraw is a no-op validation stub in this build.
	405	+	// It does NOT provide any synchronization or guarantee that widget draw operations are complete.
	406	+	// In the stub, it only validates the widget state and does not wait for any operations.
	407	+	// In real GTK builds, this would wait for all pending draw operations to complete.
406	408		func WidgetWaitForDraw(widget uintptr) {
407	409		gtkTestInitOnce.Do(func() {})
408	410		widgetMu.Lock()

@@ -415,8 +417,6 @@ func WidgetWaitForDraw(widget uintptr) {
415	417		panic("GTK-CRITICAL simulation: gtk_test_widget_wait_for_draw: widget is invalid")
416	418		}
417	419		}
418		-	// In real GTK, this would wait for all pending draw operations to complete
419		-	// In our stub, we just validate the widget state
420	420		}
421	421
422	422		// IdleAdd simulates GTK's idle callback system - DEPRECATED, use WidgetWaitForDraw instead

 	return stub.bounds, true
 }
-// WidgetWaitForDraw ensures widget operations are complete (GTK test pattern)
+// WidgetWaitForDraw is a no-op validation stub in this build.
-// This replaces the problematic IdleAdd pattern with proper widget synchronization
+// It does NOT provide any synchronization or guarantee that widget draw operations are complete.
+// In the stub, it only validates the widget state and does not wait for any operations.
+// In real GTK builds, this would wait for all pending draw operations to complete.
 func WidgetWaitForDraw(widget uintptr) {
 	gtkTestInitOnce.Do(func() {})
 	widgetMu.Lock()
 			panic("GTK-CRITICAL simulation: gtk_test_widget_wait_for_draw: widget is invalid")
 		}
 	}
-	// In real GTK, this would wait for all pending draw operations to complete
-	// In our stub, we just validate the widget state
 }
 // IdleAdd simulates GTK's idle callback system - DEPRECATED, use WidgetWaitForDraw instead