gp-auto-list-field/gpalf-count-only-non-blank-rows.js (5)

1-11: Documentation looks good but could be improved.

The header comment clearly explains the purpose of the snippet, which matches the PR objectives of counting only non-blank rows in a List field. However, the title "Dynamic Row Labels for List Fields" doesn't accurately reflect the functionality being added, which is about counting non-blank rows.

-/**
- * Gravity Perks // Auto List Field // Dynamic Row Labels for List Fields
+/**
+ * Gravity Perks // Auto List Field // Count Only Non-Blank Rows
 * https://gravitywiz.com/documentation/gravity-forms-auto-list-field/
 *
 * Count only non-blank rows in a List field.
 *
 * Instructions:
 *
 * 1. Install this snippet with our free Custom JavaScript plugin.
 *    https://gravitywiz.com/gravity-forms-code-chest/
 */

---

12-15: Consider adding jQuery dependency check.

The code uses jQuery but doesn't verify its availability. It's also missing comments explaining what the filter does and the structure of the match parameter.

+/**
+ * Filter the merge tag value before calculation.
+ * Modifies the behavior of :count modifier to only count non-blank rows in a list field.
+ * 
+ * @param {string|number} value - The current value of the merge tag
+ * @param {Array} match - The regex match array from the merge tag pattern
+ * @param {boolean} isVisible - Whether the field is visible
+ * @param {object} formulaField - The field object containing the formula
+ * @param {number} formId - The ID of the current form
+ * @return {string|number} - The modified value or original value if conditions aren't met
+ */
 gform.addFilter('gform_merge_tag_value_pre_calculation', function (value, match, isVisible, formulaField, formId) {
+    // Ensure jQuery is available
+    if (typeof jQuery === 'undefined') {
+        console.error('jQuery is required for the non-blank rows counter to work.');
+        return value;
+    }
+    
+    // Check if this is a :count merge tag
 	if (typeof match[3] === 'undefined' || match[3].indexOf(':count') === -1) {
 		return value;
 	}

---

17-23: Add error handling for invalid field ID.

The code is well-structured but lacks error handling for when the inputId doesn't convert to a valid numeric fieldId.

 	var inputId = match[1];
 	var fieldId = parseInt(inputId, 10);
+
+	// Check if the parsed fieldId is a valid number
+	if (isNaN(fieldId)) {
+		console.warn('Invalid field ID in merge tag:', inputId);
+		return value;
+	}
 
 	var $fieldWrapper = $(`#gform_${formId} #field_${formId}_${fieldId}`);
 	if ($fieldWrapper.length === 0) {
+		console.debug(`Field wrapper not found for field ID ${fieldId} in form ${formId}`);
 		return value;
 	}

---

25-28: Look for all types of form controls, not just inputs.

The code currently only checks for list groups but doesn't validate if this is actually a list field before proceeding.

 	var $rows = $fieldWrapper.find('.gfield_list_group');
 	if ($rows.length === 0) {
+		// Check if this is actually a list field
+		if (!$fieldWrapper.hasClass('gfield_contains_list')) {
+			console.debug(`Field ${fieldId} in form ${formId} is not a list field`);
+		} else {
+			console.debug(`No list rows found for field ${fieldId} in form ${formId}`);
+		}
 		return value;
 	}

---

47-48: Function return value lacks type coercion.

The return value should match the expected type of the original value for consistency. The merge tag system likely expects a string, but this returns a number.

-	return nonBlankCount;
+	// Convert to string to match the expected return type of merge tags
+	return String(nonBlankCount);
 });

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro