#2878 - Group Genmapper

[kodinkat]

• fixes: Group Genmapper #2878

---

[kodinkat]

@corsacca review ready...

[corsacca]

thanks @kodinkat!

Would you be able to check why the location dropdown get hidden?

Also please evaluate the following:

1. XSS risk in mapbox token injection
fieldHtml = fieldHtml.replace(
/mapbox-token="[^"]*"/,
mapbox-token="${window.lodash.escape(mapboxKey)}",
);
Using string .replace() on rendered HTML to inject attribute values is fragile. If renderField
ever changes its output format (e.g., single quotes, no quotes, different attribute order), this
silently breaks. Consider setting the attribute on the DOM element after insertion instead:
el.setAttribute('mapbox-token', mapboxKey);

2. alert() for validation — genmap-tile.js
alert('Name is required');
This is a browser-native alert() — it blocks the thread and isn't translatable. The rest of the
file uses window.dtGroupGenmap.strings for i18n. This string should be added to the strings
array in PHP and referenced here.
-> make translatable

3. No loading/disabled state on submit
The submit button (#group_genmap_add_child_but) is not disabled during the API call. A user can
double-click and create duplicate child groups. The original code had the same issue, but since
the form is now richer and slower to fill, it's more likely users will notice the lag and click
again.
-> make the button a loader button (like existing examples), with active class

[github-actions]

Code Review

Good work addressing the previous reviewer's feedback — the mapbox token injection via setAttribute, loading/disabled state on the submit button, and translatable strings for validation messages are all well done.

A few things worth looking at before merge:

---

Moderate Issues

1. Redundant (and overwritten) parent connection

In handleAddChild(), fields.parent_groups is explicitly set:

fields.parent_groups = { values: [{ value: parentId }] };

But additional_meta.created_from + add_connection: 'child_groups' is also sent. Looking at dt-posts.php line 172, the additional_meta processing overwrites $fields[$connection_field] — which is parent_groups. So the explicit parent_groups value from JS is silently clobbered before it ever gets used. This is harmless in the current case (both set the same value), but it's dead code that could mislead future developers into thinking the explicit field is necessary or would be respected. Consider removing the explicit parent_groups assignment and relying solely on additional_meta, which is the existing pattern from before this PR.

2. user_select field values not collected

handleAddChild() collects values by querying for web component elements:

modalContent.find('dt-text, dt-textarea, dt-number, ..., dt-location-map, dt-multi-text')

However, SHAREDFUNCTIONS.renderField() renders user_select fields as legacy typeahead HTML (a plain <input> inside <div class="dt_user_select">), not a web component. If a user_select field (e.g. assigned_to) is marked in_create_form => true for groups, it will be rendered in the form but its value will never be collected. Either add user_select value collection, or explicitly skip rendering user_select fields in the create form (return null from the renderField switch or filter them out in PHP).

---

Minor Issues

3. parentId is a string but connection field values should be integers

jQuery('#...post_id').val() returns a string, and it's passed directly as:

{ values: [{ value: parentId }] }

This only affects the (currently dead) code path mentioned in issue #1, but if the logic ever changes to use this explicit field value, a type mismatch could cause an API error. Use parseInt(parentId, 10) if the field is kept.

4. Overly complex error message construction

const msg = (template && template.replace('%s', error.message || unknownErr)) || ('Error creating child group: ' + (error.message || unknownErr));

template.replace() always returns a string (never falsy), so the || fallback is unreachable. Simplify to:

const msg = template.replace('%s', error.message || unknownErr);

5. Double focus on modal open

There's a setTimeout focus inside showAddChildModal and also the existing open.zf.reveal event handler at the bottom of the file that focuses #group_genmap_add_child_title. These will both fire. The setTimeout approach is slightly more brittle (arbitrary 100ms delay). Consider removing one, or consolidating both into the open.zf.reveal handler.

---

Positive Notes

• Addressing all three previous reviewer concerns (mapbox setAttribute, i18n for validation, loading state) is well done
• The _title sentinel key approach is acknowledged in comments and works correctly
• Proper window.lodash.escape() usage throughout the HTML building
• The PHP field filtering logic is clear and follows existing patterns
• Using overflow-x: visible via setAttribute after DOM insertion is the right approach for the location dropdown fix

[github-actions]

Code Review

The previous reviewer feedback has been well-addressed: mapbox token injection now uses setAttribute after DOM insertion, the submit button has a loading/disabled state, and validation strings are translatable. The overall approach of dynamically rendering in_create_form fields in the "Add Child" modal using DT web components is clean.

A few remaining items:

---

Issues

1. Conflicting overflow values on modal content

In showAddChildModal, after opening the modal:

content.css('overflow-x', 'visible').css('overflow-y', 'auto');

Per the CSS2.1 spec, when one overflow axis is non-visible, the other axis is computed as auto — so setting overflow-x: visible alongside overflow-y: auto on the same element results in the browser silently treating overflow-x as auto. This line has no practical effect.

The actual fix for the location dropdown clipping is the CSS rule in genmap-d3.css:

#template_metrics_modal_content .form-field-location {
    overflow: visible;
}

That targets the field wrapper div directly, which is correct. The jQuery line can be removed to avoid confusion.

2. Silent field omission when SHAREDFUNCTIONS.renderField is unavailable

Non-title fields are only rendered inside if (window.SHAREDFUNCTIONS && window.SHAREDFUNCTIONS.renderField && ...). If SHAREDFUNCTIONS is not present, only the title field gets a fallback input — all other in_create_form fields are silently dropped. Likely fine in practice, but a console.warn in the fallback branch would help during development.

3. Removed "Generation" section from group popover (undocumented)

The diff removes this block from buildPopoverHtml:

// Generation
if (data.content) {
  html += '<div class="popover-field">';
  html += '<strong>Generation:</strong>';
  ...
}

This change is not mentioned in the PR description or the referenced issue. Was this intentional? If data.content does not actually contain the generation number, a brief comment explaining the removal would help future reviewers.

---

Minor

4. _title sentinel key pollutes fieldSettings

Using fieldSettings._title as a sentinel to pass title field settings into JS while excluding it from the fieldsToRender loop is functional but mixes transport data with control logic. A cleaner approach would be a separate top-level property in the wp_localize_script payload (e.g., titleFieldSetting: {...}). Minor design point — not a blocker.

5. Redundant Number.isNaN guard

const parentId = parseInt(jQuery('...').val(), 10);
if (\!postType || \!parentId || Number.isNaN(parentId)) { return; }

parseInt returns NaN for invalid input, and \!NaN is true, so Number.isNaN(parentId) is already covered by \!parentId. Not a bug, just slightly redundant.

---

Positive Notes

• Field filtering in PHP is clear and follows existing patterns
• class_exists( 'DT_Mapbox_API' ) check before calling get_key() is good defensive coding
• parseInt(..., 10) used correctly for the post ID
• All user-facing strings are properly translatable with the disciple_tools text domain
• window.lodash.escape() applied consistently throughout HTML building

[github-actions]

Code Review

This PR addresses issue #2878 by enriching the "Add Child" modal in the generational map to support configurable in_create_form fields. The implementation is generally solid. All three concerns from the previous review have been addressed.

---

Previous Review Items Resolved

1. Mapbox token injection — now correctly uses setAttribute on the DOM element after insertion instead of HTML string replacement.
2. Translatable validation strings — name_required and error_creating_child added to PHP wp_localize_script and referenced in JS.
3. Button loading state — button is disabled with .loading class during the API call and re-enabled in all three exit paths (success, error, and API-unavailable).

---

Issues to Address

1. PHP: in_create_form === false check is unreachable (minor)

The early continue for in_create_form === false is redundant — \!empty(false) is already false, so the field would never be included by the check below it. No bug, but the guard can be removed to simplify the logic.

---

2. PHP: Array form of in_create_form not validated against current post type

if ( $field_setting['in_create_form'] === true || is_array( $field_setting['in_create_form'] ) ) {
    $create_form_fields[ $field_key ] = $field_setting;
}

When in_create_form is an array it typically lists the post types for which the field should appear. The current code includes the field regardless of whether groups is in that array. This could expose unintended fields in the Add Child modal. Consider adding in_array( $post_type, $field_setting['in_create_form'] ) to the condition.

---

3. JS: Redundant title/name exclusions in the fieldsToRender loop

title and name are already excluded when fieldsToRender is constructed, so the identical guard inside the subsequent forEach is dead code and can be removed.

---

4. Pre-existing: statusColor injected into inline CSS without sanitization

data.statusColor is interpolated directly into a style attribute without validation. A malformed value from the server could break the style block. A guard like /^#[0-9a-fA-F]{3,6}$/.test(statusColor) before use (falling back to the default) would close this. This predates the PR but the surrounding lines were touched, making it a good opportunity to address.

---

5. JS: Hard-coded web component tag-name list for field collection

The modalContent.find('dt-text, dt-textarea, ...') selector will silently miss any component types added in the future. Selecting by a name attribute that starts with the field prefix, or by a shared attribute/class consistently set by renderField, would be more resilient.

---

Minor Nit

content.find('dt-location-map') token assignment runs after jQuery(modal).foundation('open'). Setting the attribute on a custom element after insertion is functionally fine (it triggers the attribute-changed callback), but if a race condition is ever observed in practice, moving the setAttribute call to before foundation('open') would be the safer order.

---

Overall the implementation is well-structured, fallback handling is thorough, and lodash.escape is applied consistently. Item 2 (array in_create_form post-type check) is the most impactful issue to address before merge.

[github-actions]

Code Review

This PR does a solid job of addressing the feedback from the previous review. The three main issues raised (XSS risk, untranslatable strings, missing loading state) are all properly resolved. Here's a breakdown:

What's done well

• XSS fix: The mapbox token is now injected via this.setAttribute('mapbox-token', mapboxKey) after DOM insertion rather than string replacement. This is the right approach.
• i18n: All previously hardcoded strings ('Name is required', 'Error creating child group: %s', 'Close', 'Type', 'Status', etc.) are now PHP-translatable and referenced in JS via dtGroupGenmap.strings.
• Loading state: Submit button is correctly disabled + given a loading class during the API call, and re-enabled in all branches (then, catch, and the no-API fallback).
• Field collection: The multi-field collection with ComponentService.convertValue and type-based API formatting is well-structured, with good null guards and fallbacks throughout.
• parentId as integer: parseInt(..., 10) is correct—NaN is falsy so the !parentId guard handles malformed values cleanly.

---

Issues to address

1. alert() is still used for validation and error feedback

Using alert() blocks the main thread and is a poor UX pattern (especially for a modal-within-a-page flow). The modal is Foundation-based, so there is already an established pattern for showing inline errors. Consider rendering an error message inside the modal content area instead:

modalContent.prepend('<div class="alert callout">' + window.lodash.escape(msg) + '</div>');

The translatable strings (name_required, error_creating_child) are correctly wired up—it's just the delivery mechanism that's still blocking.

---

2. Redundant title/name filtering in JS

fieldsToRender is built by already excluding title and name, but the iteration immediately re-checks those same keys and returns early. The inner guard is unreachable dead code. Minor, but worth removing to avoid confusion for future readers.

---

3. PHP field filter: user_select skip is broader than intended

if ( isset( $field_setting['type'] ) && $field_setting['type'] === 'user_select' ) {
    continue;
}

The comment says this targets assigned_to specifically, but the implementation silently skips all user_select fields—including any custom ones a site has explicitly set in_create_form => true on. Consider checking for the specific key (assigned_to), or at least only skipping when in_create_form is not explicitly set:

if (
    isset( $field_setting['type'] ) && $field_setting['type'] === 'user_select'
    && empty( $field_setting['in_create_form'] )
) {
    continue;
}

---

4. CSS rule targets a shared modal selector

#template_metrics_modal_content .form-field-location {
    overflow: visible;
}

#template_metrics_modal_content is shared across all metrics modals. If another modal ever renders content inside .form-field-location, this rule will silently affect it. A more tightly scoped selector (e.g., one scoped to a class added only to the genmapper modal) would be safer long-term.

---

Minor / Nits

• window.lodash.escape() on PHP translation strings coming from dtGroupGenmap.strings is safe but slightly over-engineered—wp_localize_script JSON-encodes the values, so they arrive as plain JS strings. Escaping them again before injecting into .innerHTML is the right call; applying it in both places is harmless but redundant.
• The modal max-height bump from 300px → 400px is reasonable for the richer form, though max-height: min(400px, 80vh) would degrade more gracefully on shorter viewports.

---

Summary

The core fixes (XSS, i18n, loading state) are solid. The main remaining items:

1. Replace alert() with inline error feedback — higher priority for UX and accessibility
2. Clean up the unreachable title/name guard in JS — low effort
3. Reconsider the blanket user_select skip in PHP — potential functional issue for sites with custom user_select fields flagged for create forms

[kodinkat]

@corsacca review ready....

[corsacca]

@kodinkat the location options list still is cut off. Do you get the same behavior?

[kodinkat]

@kodinkat the location options list still is cut off. Do you get the same behavior?

@corsacca no, I'm not seeing this? Anything in the console?

[github-actions]

Code Review

Good work on this PR! The feature addresses the issue well — the generational map's "Add Child" modal now respects the site's configured in_create_form fields and uses proper i18n throughout. A few things worth addressing:

---

Issues

1. overflow-y: visible breaks scrolling on taller modals

In buildAddChildModal(), the modal content is set to overflow-y: visible (and in genmap-d3.css as well). This is done to allow location dropdowns to extend outside the container, but it means the modal won't scroll if many fields are rendered. A user could have several in_create_form fields that push content below the max-height: 400px bound — and they would be completely unreachable.

A more robust approach would be to keep overflow-y: auto on the modal content and use overflow: visible only on the specific .form-field-location wrapper (which you're already doing in CSS). You may need to restructure the location field HTML so its overflow doesn't get clipped by overflow: auto on the parent — a common technique is to move the dropdown portal to a fixed overlay position.

2. Field key detection can silently fail in handleAddChild()

const fieldKey = component.name || component.id?.replace(fieldPrefix, '');

If a component's id does not start with fieldPrefix (e.g. if SHAREDFUNCTIONS.renderField generates a different naming convention), replace won't strip the prefix and the full id becomes the key. fieldSettings[fullId] will be undefined, so the field is silently dropped. Consider using a more robust approach, e.g. component.id?.startsWith(fieldPrefix) ? component.id.slice(fieldPrefix.length) : component.name.

3. submitBtn.attr('disabled', false) is incorrect jQuery

$el.attr('disabled', false) sets the attribute to "false" (a string) rather than removing it — so the button stays disabled. Use either:

• submitBtn.prop('disabled', false) (correct for boolean attributes), or
• submitBtn.removeAttr('disabled')

This affects all three re-enable calls in handleAddChild().

4. fieldPrefix is defined in two separate functions

const fieldPrefix = 'group_genmap_add_child_' is duplicated in both buildAddChildModal and handleAddChild. If the prefix ever changes, both places need updating. Hoist it to module scope or a shared constant so a single source of truth exists.

---

Minor Notes

• Excluding name key without comment: fieldsToRender explicitly skips key === 'name'. If no DT field should have key name (as opposed to title), a brief comment would make the intent clear. If it's a guard for legacy data, that's worth noting.

• PHP null coalesce on get_key(): DT_Mapbox_API::get_key() ?? '' — the ?? is fine, but worth verifying that get_key() doesn't return false or 0 (which ?? wouldn't catch). If those are possible, (string) cast may be preferable.

• parseInt result not re-used as integer in payload: parentId is parsed with parseInt(..., 10), which is good for the falsy check, and it ends up in additional_meta.created_from. This is fine — just confirming it's intentional.

---

What's good

• The PHP field filtering logic in base-setup.php is clean, well-commented, and correctly handles all the edge cases (hidden, in_create_form: false, in_create_form: true, in_create_form: [postType]).
• All previously hardcoded UI strings ('Type', 'Status', 'Members', 'Assigned', 'Close', 'Expand', 'Collapse') are now properly wrapped with __() and localized.
• The error_creating_child string uses %s for the error message placeholder — good for translators.
• Loading state on the submit button (disabled + .loading class) prevents double-submits — nice touch.
• Cleanup of the genmap-add-child-modal class on closed.zf.reveal is correct and avoids CSS bleed to other modal uses.
• Setting the mapbox token after DOM insertion via setAttribute is cleaner than string interpolation.

[github-actions]

PR Review: Group Genmapper (fixes issue 2878). Good overall work - the changes add dynamic field rendering to the Add Child modal, improve i18n coverage, and introduce better UX patterns. Positive: all hardcoded strings now go through wp_localize_script, submit button is properly disabled during async calls, modal class is cleaned up on close, and there are good fallbacks throughout. Issues found: (1) parseInt result guard works but isNaN check would be clearer intent; (2) alert() calls are blocking and jarring - the existing .group-genmap-message element with aria-live could be reused for inline errors; (3) hardcoded component tag name selector will silently skip new web component types added in future - a data-attribute approach would be more maintainable; (4) template.replace('%s', error.message) can corrupt output if error message contains dollar-digit sequences - use template.split('%s').join(...) instead; (5) overflow-y:visible inline style may be silently ignored if overflow-x is already set - the genmap-d3.css rule is the correct fix; (6) component.name fallback in field key resolution could mismatch if name differs from id-without-prefix. Test suggestions: test with multiple in_create_form field types, test modal re-open after failed submission, test with no Mapbox key, test RTL. Overall solid work - items 3 and 4 are most likely to cause silent bugs.

[kodinkat]

@corsacca location options no longer cut off....

[corsacca]

Thanks @kodinkat!