Harden gameplay settings JS bridge and add comprehensive tests

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,37 @@ 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)
+	if is_instance_valid(settings_res):
+		difficulty_slider.value = Globals.settings.difficulty
+		difficulty_label.text = "{" + str(Globals.settings.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 +100,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 +117,31 @@ 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
+	## Cleanup on unexpected tree exit.
 	Globals.log_message("Gameplay Settings _on_tree_exited called.", Globals.LogLevel.DEBUG)
 
-	# 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
+	# 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
@@ -261,72 +283,118 @@ 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) + "}"
+	var settings_res := Globals.settings if is_instance_valid(Globals) else null
+	if not is_instance_valid(settings_res):
+		Globals.log_message(
+			"Gameplay Settings: Globals.settings unavailable; skipping difficulty update.",
+			Globals.LogLevel.WARNING
+		)
+		return
+	# Update the resource first (this triggers clamping in the setter)
+	settings_res.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 = settings_res.difficulty
+	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
+
+	# 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
 
 	var first_arg: Variant = args[0]
+	var potential_value: Variant = null
+
+	# Refactored logic for _on_change_difficulty_js in gameplay_settings.gd
+	# 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:
+			potential_value = first_arg[0]
+		else:
+			Globals.log_message("JS callback: Array is empty.", Globals.LogLevel.WARNING)
+			return
+	elif first_arg is JavaScriptObject:
+		# For JavaScriptObject, treat it as a proxy to a JS array
+		# Use the specific JS indexing if you are certain it is a JS array,
+		# or handle it as a single-value reference.
+		# JS-FIX: If we receive a JS Object (like from Playwright),
+		# we must index it to get the raw value before the type check.
+		if first_arg.length > 0:
+			potential_value = first_arg[0]
+		else:
+			Globals.log_message("JS callback: JavaScriptObject is empty.", Globals.LogLevel.WARNING)
+			return
+	else:
+		# Handle scalar values (e.g., [1.5]) directly
+		potential_value = first_arg
+
+	# 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
+		# REMOVE 'return' to allow the value to reach the resource for clamping
+		# 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)
 
 

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()