Merge pull request #488 from ikostan/settings-labels-display-unclamped-values

Harden gameplay settings JS bridge and add comprehensive tests

Author: ikostan

scripts/gameplay_settings.gd

@@ -6,7 +6,7 @@ extends Control
 ## actual engine singletons.
 var js_bridge_wrapper: JavaScriptBridgeWrapper = JavaScriptBridgeWrapper.new()
 var os_wrapper: OSWrapper = OSWrapper.new()
-var js_window: JavaScriptObject
+var js_window: Variant
 var _change_difficulty_cb: JavaScriptObject
 var _gameplay_back_button_pressed_cb: JavaScriptObject
 var _gameplay_reset_cb: JavaScriptObject
@@ -27,22 +27,39 @@ func _ready() -> void:
 	# Configure for web overlays (invisible but positioned)
 	process_mode = Node.PROCESS_MODE_ALWAYS  # Ignore pause
 
-	difficulty_slider.value_changed.connect(_on_difficulty_value_changed)
-	# Set initial difficulty label (sync with global)
-	difficulty_slider.value = Globals.settings.difficulty
-	difficulty_label.text = "{" + str(Globals.settings.difficulty) + "}"
+	var settings_res := Globals.settings if is_instance_valid(Globals) else null
+
+	# ADD GUARDS HERE:
+	if not difficulty_slider.value_changed.is_connected(_on_difficulty_value_changed):
+		difficulty_slider.value_changed.connect(_on_difficulty_value_changed)
+
+	# Set initial difficulty label (sync with global if available)
+	# FIX: Use the local reference for consistency
+	if is_instance_valid(settings_res):
+		difficulty_slider.value = settings_res.difficulty
+		difficulty_label.text = "{" + str(settings_res.difficulty) + "}"
+	else:
+		difficulty_slider.value = _default_difficulty
+		difficulty_label.text = "{" + str(_default_difficulty) + "}"
+
 	# Back button
 	if not gameplay_back_button.pressed.is_connected(_on_gameplay_back_button_pressed):
 		gameplay_back_button.pressed.connect(_on_gameplay_back_button_pressed)
 	# Reset button listener
 	if not gameplay_reset_button.pressed.is_connected(_on_gameplay_reset_button_pressed):
 		gameplay_reset_button.pressed.connect(_on_gameplay_reset_button_pressed)
 	# NEW: Attach tree_exited for unexpected removal cleanup (like other settings scripts)
-	tree_exited.connect(_on_tree_exited)
+	if not tree_exited.is_connected(_on_tree_exited):
+		tree_exited.connect(_on_tree_exited)
 
 	# NEW: The UI now observes the resource for external changes
-	if not Globals.settings.setting_changed.is_connected(_on_external_setting_changed):
-		Globals.settings.setting_changed.connect(_on_external_setting_changed)
+	# if not Globals.settings.setting_changed.is_connected(_on_external_setting_changed):
+	#	Globals.settings.setting_changed.connect(_on_external_setting_changed)
+	if (
+		is_instance_valid(settings_res)
+		and not settings_res.setting_changed.is_connected(_on_external_setting_changed)
+	):
+		settings_res.setting_changed.connect(_on_external_setting_changed)
 
 	if os_wrapper.has_feature("web"):
 		# Toggle overlays...
@@ -85,10 +102,15 @@ func _ready() -> void:
 
 
 func _on_external_setting_changed(setting_name: String, new_value: Variant) -> void:
+	## SYNC UI ONLY:
+	## This observer reacts to changes from the resource.
+	## We must ensure the UI nodes are still valid before updating them.
 	if setting_name == "difficulty":
-		# SYNC UI ONLY:
-		# The resource has already been updated, so we only need to update the UI components.
-		# Use set_value_no_signal to prevent re-triggering the local _on_difficulty_value_changed handler.
+		# FIX: Guard against 'previously freed' errors during teardown/unit tests
+		if not is_instance_valid(difficulty_slider) or not is_instance_valid(difficulty_label):
+			return
+
+		# Use set_value_no_signal to prevent re-triggering local handlers
 		difficulty_slider.set_value_no_signal(float(new_value))
 		difficulty_label.text = "{" + str(new_value) + "}"
 
@@ -97,29 +119,34 @@ func _on_tree_exited() -> void:
 	## Cleanup on unexpected tree exit (e.g. parent removed without calling back button).
 	## Disconnects signals, restores previous menu if not intentional, clears JS/DOM state.
 	## :rtype: void
-	Globals.log_message("Gameplay Settings _on_tree_exited called.", Globals.LogLevel.DEBUG)
+	## Cleanup on unexpected tree exit.
 
-	# Disconnect the global resource observer to prevent stale references
-	# GUARD: Ensure Globals and the settings resource are still valid before disconnecting
-	# Use a local variable to safely check and access the settings resource
-	var settings_res := Globals.settings if is_instance_valid(Globals) else null
+	# FIX: Guard the initial log message against a torn-down Globals singleton
+	if is_instance_valid(Globals):
+		Globals.log_message("Gameplay Settings _on_tree_exited called.", Globals.LogLevel.DEBUG)
 
+	# 1. Safe Global Resource Disconnection
+	var settings_res := Globals.settings if is_instance_valid(Globals) else null
 	if is_instance_valid(settings_res):
 		if settings_res.setting_changed.is_connected(_on_external_setting_changed):
 			settings_res.setting_changed.disconnect(_on_external_setting_changed)
 
-	# Disconnect Godot signals if still connected
-	if difficulty_slider.value_changed.is_connected(_on_difficulty_value_changed):
-		difficulty_slider.value_changed.disconnect(_on_difficulty_value_changed)
-	if gameplay_back_button.pressed.is_connected(_on_gameplay_back_button_pressed):
-		gameplay_back_button.pressed.disconnect(_on_gameplay_back_button_pressed)
-	if gameplay_reset_button.pressed.is_connected(_on_gameplay_reset_button_pressed):
-		gameplay_reset_button.pressed.disconnect(_on_gameplay_reset_button_pressed)
+	# 2. FIX: Guarded Local Disconnections
+	# We must check if the nodes still exist before accessing 'value_changed' or 'pressed'
+	if is_instance_valid(difficulty_slider):
+		if difficulty_slider.value_changed.is_connected(_on_difficulty_value_changed):
+			difficulty_slider.value_changed.disconnect(_on_difficulty_value_changed)
 
-	# Clean up JS callbacks on window object
-	_unset_gameplay_settings_window_callbacks()
+	if is_instance_valid(gameplay_back_button):
+		if gameplay_back_button.pressed.is_connected(_on_gameplay_back_button_pressed):
+			gameplay_back_button.pressed.disconnect(_on_gameplay_back_button_pressed)
+
+	if is_instance_valid(gameplay_reset_button):
+		if gameplay_reset_button.pressed.is_connected(_on_gameplay_reset_button_pressed):
+			gameplay_reset_button.pressed.disconnect(_on_gameplay_reset_button_pressed)
 
-	# Null out stored callback references
+	# 3. Clean up JS/Web state
+	_unset_gameplay_settings_window_callbacks()
 	_change_difficulty_cb = null
 	_gameplay_back_button_pressed_cb = null
 	_gameplay_reset_cb = null
@@ -133,7 +160,12 @@ func _on_tree_exited() -> void:
 			document.getElementById('gameplay-reset-button').style.display = 'none';
 			"""
 
-		if not _intentional_exit and not Globals.hidden_menus.is_empty():
+		# FIX: Guard the hidden_menus array check against a torn-down Globals singleton
+		if (
+			not _intentional_exit
+			and is_instance_valid(Globals)
+			and not Globals.hidden_menus.is_empty()
+		):
 			# Unexpected exit → restore previous menu and options overlays
 			var prev_menu: Node = Globals.hidden_menus.pop_back()
 			if is_instance_valid(prev_menu):
@@ -261,75 +293,132 @@ func _on_difficulty_value_changed(value: float) -> void:
 	## :type value: float
 	## :rtype: void
 	# Update the resource first (this triggers clamping in the setter)
-	Globals.settings.difficulty = value
+	#Globals.settings.difficulty = value
 	# Update the UI components using the ALREADY CLAMPED value from the resource
-	difficulty_slider.value = Globals.settings.difficulty
-	difficulty_label.text = "{" + str(Globals.settings.difficulty) + "}"
+	#difficulty_slider.value = Globals.settings.difficulty
+	#difficulty_label.text = "{" + str(Globals.settings.difficulty) + "}"
+	var settings_res := Globals.settings if is_instance_valid(Globals) else null
+
+	# FIX: Use the local reference exclusively
+	if not is_instance_valid(settings_res):
+		Globals.log_message(
+			"Gameplay Settings: settings_res unavailable; skipping difficulty update.",
+			Globals.LogLevel.WARNING
+		)
+		return
+
+	settings_res.difficulty = value
+	if is_instance_valid(difficulty_slider):
+		difficulty_slider.set_value_no_signal(settings_res.difficulty)
+	if is_instance_valid(difficulty_label):
+		difficulty_label.text = "{" + str(settings_res.difficulty) + "}"
 
 
 # New: JS-specific callback (exactly one Array arg, no default)
 func _on_change_difficulty_js(args: Array) -> void:
 	## JS callback for changing difficulty.
 	##
-	## Routes to the signal handler.
+	## Routes to the signal handler after performing strict type and
+	## bounds validation to prevent engine crashes on malformed JS input.
 	##
 	## :param args: Array containing the value (from JS).
 	## :type args: Array
 	## :rtype: void
-	if args.is_empty():
-		Globals.log_message(
-			"JS difficulty callback received empty args—skipping.", Globals.LogLevel.WARNING
-		)
+
+	var potential_value: Variant = _extract_js_difficulty(args)
+
+	if potential_value == null:
 		return
 
-	var first_arg: Variant = args[0]
+	# GS-JS-12/15/22: Validate that the extracted value is a convertible type
 	if (
-		first_arg is not JavaScriptObject
-		and typeof(first_arg) != TYPE_ARRAY
-		and first_arg.size() == 0
-		and first_arg.is_empty()
+		typeof(potential_value) != TYPE_INT
+		and typeof(potential_value) != TYPE_FLOAT
+		and typeof(potential_value) != TYPE_STRING
 	):
 		Globals.log_message(
-			(
-				"JS difficulty callback received invalid first arg (not a non-empty array): "
-				+ str(args)
-			),
+			"JS difficulty callback received non-convertible value: " + str(potential_value),
 			Globals.LogLevel.WARNING
 		)
 		return
 
-	var potential_value: Variant = first_arg[0]
-	if (
-		typeof(potential_value) != TYPE_INT
-		and typeof(potential_value) != TYPE_FLOAT
-		and typeof(potential_value) != TYPE_STRING
-	):
+	# GS-JS-03: Coerce to float (e.g., "1.5" becomes 1.5)
+	# FIX: Ensure strings are numeric before conversion to prevent 0.0/clamping reset
+	if typeof(potential_value) == TYPE_STRING and not potential_value.is_valid_float():
 		Globals.log_message(
-			"JS difficulty callback received non-convertible value: " + str(args),
+			"JS difficulty callback: Rejected non-numeric string: " + str(potential_value),
 			Globals.LogLevel.WARNING
 		)
 		return
 
 	var value: float = float(potential_value)
+
+	# GS-JS-30: Guard against missing UI nodes during callback
+	if not is_instance_valid(difficulty_slider):
+		Globals.log_message(
+			"JS difficulty callback: Slider node is invalid/freed.", Globals.LogLevel.WARNING
+		)
+
+		# FIX: Safely check for Globals and Settings before falling back
+		var settings_res := Globals.settings if is_instance_valid(Globals) else null
+		if is_instance_valid(settings_res):
+			settings_res.difficulty = value  # Update resource even if UI is gone
+		return
+
+	# GS-JS-04/05: Validate bounds against the UI constraints
 	if value < difficulty_slider.min_value or value > difficulty_slider.max_value:
 		Globals.log_message(
-			(
-				"JS difficulty callback received out-of-bounds value: "
-				+ str(value)
-				+ " (args: "
-				+ str(args)
-				+ ")"
-			),
+			"JS difficulty callback received out-of-bounds value: " + str(value),
 			Globals.LogLevel.WARNING
 		)
-		return
 
 	Globals.log_message(
 		"JS difficulty callback called with valid value: " + str(value), Globals.LogLevel.DEBUG
 	)
+
+	# Pass the validated value to the standard handler
 	_on_difficulty_value_changed(value)
 
 
+## GS-JS: Helper to extract a potential value from diverse JS bridge payloads.
+## Isolates branching logic for standard Arrays, JavaScriptObjects, and scalars.
+func _extract_js_difficulty(args: Array) -> Variant:
+	# GS-JS-10: Guard against entirely empty arguments from the bridge
+	if args.is_empty():
+		Globals.log_message(
+			"JS difficulty callback received empty args—skipping.", Globals.LogLevel.WARNING
+		)
+		return null
+
+	var first_arg: Variant = args[0]
+
+	# GS-JS-20/21: Branch logic to handle TYPE_ARRAY and JavaScriptObject separately
+	if typeof(first_arg) == TYPE_ARRAY:
+		# Safe to use .size() and indexing on standard GDScript Arrays
+		if first_arg.size() > 0:
+			return first_arg[0]
+
+		Globals.log_message("JS callback: Array is empty.", Globals.LogLevel.WARNING)
+		return null
+
+	if first_arg is JavaScriptObject:
+		# BUG RISK FIX: Validate the 'length' property exists and is numeric
+		# before treating the object as an array.
+		# Note: Must use dot notation, as .get() attempts to call a JS method.
+		var js_length: Variant = first_arg.length
+
+		if js_length != null and typeof(js_length) in [TYPE_INT, TYPE_FLOAT] and js_length > 0:
+			# JS-FIX: If we receive a JS Object (like from Playwright),
+			# we must index it to get the raw value before the type check.
+			return first_arg[0]
+
+		# It is a generic JS object or a non-array; treat as a scalar reference
+		return first_arg
+
+	# Handle scalar values (e.g., [1.5]) directly
+	return first_arg
+
+
 ## Grabs initial focus on the difficulty slider using the global helper.
 ## Ensures the slider is focused when the menu opens.
 ## Falls back to other controls if needed.

scripts/globals.gd

@@ -51,12 +51,12 @@ func _ready() -> void:
 	log_message("Log level set to: " + LogLevel.keys()[settings.current_log_level], LogLevel.DEBUG)
 	_load_settings()  # Load persisted settings first
 
-	# Connect to the resource signal to centralize side effects [cite: 151]
+	# Connect to the resource signal to centralize side effects
 	if settings:
 		settings.setting_changed.connect(_on_setting_changed)
 
 
-## Reactive handler for the Observer Pattern [cite: 141]
+## Reactive handler for the Observer Pattern
 func _on_setting_changed(setting_name: String, new_value: Variant) -> void:
 	# Skip persistence and logging if we are in a bulk-loading state
 	if _is_loading_settings:
@@ -65,10 +65,10 @@ func _on_setting_changed(setting_name: String, new_value: Variant) -> void:
 	# FIX: Ensure we are comparing String to String or using correct types
 	var log_msg: String = "Setting '%s' updated to: %s" % [setting_name, str(new_value)]
 
-	# Automatically log the change [cite: 59]
+	# Automatically log the change
 	log_message(log_msg, LogLevel.DEBUG)
 
-	# Automatically persist to disk [cite: 53]
+	# Automatically persist to disk
 	_save_settings()